Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add NRT for the whole client assembly #1626

Merged
merged 14 commits into from
Jul 9, 2024
Merged

Add NRT for the whole client assembly #1626

merged 14 commits into from
Jul 9, 2024

Conversation

bollhals
Copy link
Contributor

@bollhals bollhals commented Jul 3, 2024 •
edited by lukebakken
Loading

Proposed Changes

Fixes #1596
Fixes #1622

Enables Nullable Reference Types in the client assembly

I realize a bit late that you intended to do this in 8.0 ... unfortunately I already was 80% through when I did see it 💯

You'll have to decide whether it's worth including it or push it after the snap for 7.0

Types of Changes

  • Bug fix (non-breaking change which fixes issue #NNNN)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause an observable behavior change in existing systems)
  • Documentation improvements (corrections, new content, etc)
  • Cosmetic change (whitespace, formatting, etc)

Checklist

  • I have read the CONTRIBUTING.md document
  • I have signed the CA (see https://cla.pivotal.io/sign/rabbitmq)
  • All tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • Any dependent changes have been merged and published in related repositories

Further Comments

How does it work with the PublicAPI thing, does this need adjustment as well?

bollhals
bollhals commented Jul 3, 2024
View reviewed changes
projects/Directory.Packages.props Show resolved Hide resolved
projects/RabbitMQ.Client/RabbitMQ.Client.csproj Show resolved Hide resolved
projects/RabbitMQ.Client/client/api/BinaryTableValue.cs Show resolved Hide resolved
projects/RabbitMQ.Client/client/api/IChannel.cs Show resolved Hide resolved
projects/RabbitMQ.Client/client/api/TcpClientAdapter.cs Show resolved Hide resolved
projects/RabbitMQ.Client/client/impl/ChannelBase.cs Show resolved Hide resolved
projects/RabbitMQ.Client/client/impl/Connection.cs Show resolved Hide resolved
projects/RabbitMQ.Client/client/impl/SocketFactory.cs Show resolved Hide resolved
projects/RabbitMQ.Client/client/impl/SocketFrameHandler.cs Show resolved Hide resolved
projects/RabbitMQ.Client/client/impl/SocketFrameHandler.cs Show resolved Hide resolved
@bollhals
Copy link
Contributor Author

bollhals commented Jul 4, 2024

@lukebakken Any idea what I broke with the public api analyzer that it reports internals on the netstandard2.0 target?

@lukebakken
Copy link
Contributor

@bollhals somehow this check started to work:

dotnet_diagnostic.RS0051.severity = error

I disabled it (https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/Microsoft.CodeAnalysis.PublicApiAnalyzers.md#rs0051-add-internal-types-and-members-to-the-declared-api)

@lukebakken lukebakken marked this pull request as ready for review July 8, 2024 15:22
@lukebakken lukebakken added this to the 7.0.0 milestone Jul 8, 2024
@lukebakken lukebakken self-assigned this Jul 8, 2024
@lukebakken lukebakken requested review from lukebakken and lechu445 July 8, 2024 17:05
@lukebakken lukebakken requested a review from lechu445 July 9, 2024 00:20
@lukebakken
Copy link
Contributor

@bollhals @lechu445 - anything else? I plan to merge this by 0900PDT today.

@lechu445
Copy link
Contributor

lechu445 commented Jul 9, 2024

@lukebakken nothing more from my side. LGTM

@bollhals
Copy link
Contributor Author

bollhals commented Jul 9, 2024

@bollhals @lechu445 - anything else? I plan to merge this by 0900PDT today.

With your recent commit, not from my side.
Hope it is all annotated corectly 🤞

@lukebakken
Copy link
Contributor

@bollhals @lechu445 @paulomorgado thank you so much for your insights and contributions here 🥇

@lukebakken lukebakken merged commit 0c8492a into rabbitmq:main Jul 9, 2024
11 checks passed
@bollhals bollhals deleted the NRT branch July 9, 2024 21:04
@ngbrown

This comment was marked as resolved.

Sign in to view
@lukebakken
Copy link
Contributor

@ngbrown it appears to be converted - https://github.com/rabbitmq/rabbitmq-dotnet-client/blob/main/projects/RabbitMQ.Client/Impl/RabbitMQActivitySource.cs

@ngbrown
Copy link
Contributor

ngbrown commented Sep 27, 2024

@lukebakken yes, you are correct. I had thought I had copied out the most recent version, but evidently not. Sorry about bothering you.

@lukebakken
Copy link
Contributor

@ngbrown no worries at all! If you're testing out the v7 RC candidates, we would appreciate any feedback you have. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lechu445 lechu445 lechu445 left review comments

@paulomorgado paulomorgado paulomorgado left review comments

@lukebakken lukebakken lukebakken approved these changes

Assignees

@lukebakken lukebakken

Labels
None yet
Projects
None yet
Milestone
7.0.0
Development

Successfully merging this pull request may close these issues.

NullReferenceException when setting null Uri into ConnectionFactory Nullable Reference Types
5 participants
@bollhals @lukebakken @lechu445 @ngbrown @paulomorgado