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

Enable nullable globally and fix issues #21

Merged
merged 8 commits into from
Apr 17, 2023
Merged

Enable nullable globally and fix issues #21

merged 8 commits into from
Apr 17, 2023

Conversation

erikmav
Copy link
Contributor

@erikmav erikmav commented Mar 25, 2023

For #19 . Remove some ArgumentNullException where nullability covers it to reduce runtime code (this is a change to public surface area, can be reverted - see changes in unit tests). Some opportunistic code cleanup ahead of #9 .

erikmav added 6 commits March 25, 2023 13:54
…ption where nullability covers it to reduce runtime code (this is a change to public surface area - can be reverted). Some opportunistic code cleanup.
Derrick-
Derrick- previously approved these changes Apr 3, 2023
Copy link
Contributor

@Derrick- Derrick- left a comment

Choose a reason for hiding this comment

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

This is great work. Takes care of a lot of null issues, Upgrades string.Format to string interpolation. Eliminates some unnecessary accessors.

It's a .Net version upgrade: most of the null features are introduced in C# 8.
(I had to look some of them up)

I am confident this is a good improvement to the project.

Arlodotexe
Arlodotexe previously approved these changes Apr 12, 2023
Copy link
Collaborator

@Arlodotexe Arlodotexe left a comment

Choose a reason for hiding this comment

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

There's some places we could replace null suppression with input validation (early validation makes for smaller stack traces!), but that can be saved for another time.

Fantastic PR, I reviewed every line and have other no notes. Approved.

src/IpfsCore.csproj Outdated Show resolved Hide resolved
@Arlodotexe Arlodotexe dismissed stale reviews from Derrick- and themself via 0721fb8 April 12, 2023 22:00
@Arlodotexe
Copy link
Collaborator

This will publish as soon as it merges, for use in ipfs-shipyard/net-ipfs-http-client#21

@erikmav
Copy link
Contributor Author

erikmav commented Apr 17, 2023

@Arlodotexe your review was reset so this never committed or published. When done I'll push a dependency update to ipfs-shipyard/net-ipfs-http-client#21

@Arlodotexe
Copy link
Collaborator

Whoops! Sorry about that, let's kick it off.

@Arlodotexe Arlodotexe merged commit 3450fee into ipfs-shipyard:main Apr 17, 2023
@erikmav erikmav deleted the dev/erikmav/nullable branch April 17, 2023 19:11
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.

3 participants