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.

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

Add .NET 5 target and arrange nullability attributes #636

Merged
merged 3 commits into from
Dec 10, 2020

Conversation

zvirja
Copy link
Contributor

@zvirja zvirja commented Dec 8, 2020

Decided to play a bit with nullability and enhance this project in the same time 😊 Enabled nullability checks for NET 5. As a side effect also added .NET 5 framework target.

Code was adjusted to ensure it's null-valid. I did not find any subtle bug though. Some details of the implementation:

  • Nullability is enabled for .NET 5 target only, as recommended by MS. For older versions we rely on .NET 5 build.
  • I disabled nullability annotations for the DSL related API (a.k.a. public surface). This way our clients do not have to tediously fight to the warnings they might get due to nullability. Not sure it's would be a real problem, so we could revisit this decision later.
  • member.DeclaringType was mostly null-suppressed, as I do not expect nulls there.
  • obj.ToString() type annotation allows null (against common sense IMO 😖), so I coerce value to an empty string just in case.
  • I added null guards to some of the entry points from DSL related API. That is to be able later remove arguments checks in our code via subsequent PRs.

I also plan to also make a subsequent PR and use the latest C# features in our code files. Also want to remove null checks in the code where we enabled nullability. If you have strong opinion against that please speak 😄

P.S. Also had to maintain the project (update SDK and Ruby on CI) to make sure I can build the project.

@zvirja zvirja requested a review from dtchepak December 8, 2020 17:56
@zvirja zvirja force-pushed the enable-nullability branch 7 times, most recently from dd08d72 to 624ccce Compare December 8, 2020 18:55
- Update .NET SDK on CI
- Update Ruby on CI
- Embed icon to the NuGet package
- Trim trailing whitespace
@zvirja zvirja force-pushed the enable-nullability branch from 624ccce to 6e91b62 Compare December 8, 2020 19:13
@zvirja zvirja force-pushed the enable-nullability branch from 6e91b62 to c4e9e8e Compare December 8, 2020 19:14
Copy link
Member

@dtchepak dtchepak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great @zvirja , thanks!

I've left comments but approving as they are all due to me not being familiar with non-nullable ref types yet 😂

src/NSubstitute/Core/Argument.cs Show resolved Hide resolved
src/NSubstitute/Core/IReturn.cs Outdated Show resolved Hide resolved
src/NSubstitute/Core/Maybe.cs Outdated Show resolved Hide resolved
src/NSubstitute/Core/RobustThreadLocal.cs Show resolved Hide resolved
@zvirja zvirja force-pushed the enable-nullability branch from c4e9e8e to 5cd76ab Compare December 9, 2020 12:33
@zvirja zvirja force-pushed the enable-nullability branch from 5cd76ab to 4c488c3 Compare December 9, 2020 12:37
@zvirja
Copy link
Contributor Author

zvirja commented Dec 9, 2020

@dtchepak I updated a bit of code after I found out that we could use T? for generic types. Please give it another look if you would like. Otherwise, I'll simply merge it tomorrow 😊

Copy link
Member

@dtchepak dtchepak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

Thanks answering my questions 😄

@zvirja zvirja merged commit 46d4e06 into nsubstitute:master Dec 10, 2020
@zvirja zvirja deleted the enable-nullability branch December 10, 2020 11:10
dtchepak added a commit to dtchepak/NSubstitute that referenced this pull request Jan 17, 2022
@dtchepak dtchepak mentioned this pull request Jan 17, 2022
dtchepak added a commit that referenced this pull request Jan 24, 2022
Releasing changes from #636 and #674.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants