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

guard against null #1742

Merged
merged 4 commits into from Sep 9, 2022
Merged

guard against null #1742

merged 4 commits into from Sep 9, 2022

Conversation

SimonCropp
Copy link
Contributor

No description provided.

@nblumhardt
Copy link
Member

Definitely an improvement; it's a shame we can't use ArgumentNullException.ThrowIfNull() and that there's no generic argument-returning version of that method :-(

Perhaps since our eyes are all going to get trained to glance over ArgumentNullException.ThrowIfNull() we could try aligning the naming somehow? ArgumentNullExceptionEx.ThrowIfNull() is truly horrific, but ... could maybe live with it? ANE.ThrowIfNull()? 🤔

I might need a chance to ponder a bit; agreed we should push forward in this direction somehow though 👍

@SimonCropp
Copy link
Contributor Author

note that ArgumentNullException.ThrowIfNull is not available in older frameworks. so even if it wasnt generic, we should still need to polyfill

happy to name the type+method whatever u want. it could even be an extension method. so instead of Guard.AgainstNull(scalarType); it could be be scalarType.NotNull();

@SimonCropp
Copy link
Contributor Author

i must admit i much prefer Guard.AgainstNull over ArgumentNullExceptionEx.ThrowIfNull. less characters, and reads better

@nblumhardt
Copy link
Member

😆 yeah!

I'm not so much worried about the naming as drifting from what other codebases use and what the tooling supports.

E.g. in Rider I frequently add argument null checks using the

  • "Check argument for null" and
  • "Combine null check with assignment" actions,

both of which would generate inconsistent code once we switch to Guard. We'll also have to watch out for incoming changes in PRs, or end up with an inconsistent codebase 🤔

I'd swim against the current happily if the net positive was big enough, but with some more thought, I'm not sure it adds up in this case. What do you think?

@SimonCropp
Copy link
Contributor Author

i see your concern. two points

  • this is (mostly) for public APIs. which dont change too much in a codebase
  • i can write a unit test that looks for ArgumentNullException and fails with "please use Guard.AgainstNull"

@SimonCropp
Copy link
Contributor Author

i can write a unit test that looks for ArgumentNullException and fails with "please use Guard.AgainstNull"

done

@nblumhardt
Copy link
Member

Nice! I'll loop back to this shortly. I'm wondering if we could use the same approach to enforce that we only use Guard on public APIs? 🤔

@SimonCropp
Copy link
Contributor Author

I'm wondering if we could use the same approach to enforce that we only use Guard on public APIs?

it would be possible with cecil. but IMO it is reaching the point of diminishing returns

@nblumhardt nblumhardt merged commit 4d13be5 into serilog:dev Sep 9, 2022
@SimonCropp SimonCropp deleted the Guard-against-null branch September 9, 2022 12:09
@nblumhardt nblumhardt mentioned this pull request Sep 12, 2022
Twinki14 pushed a commit to Twinki14/CitizenFX.Extensions.Client.Serilog that referenced this pull request Dec 30, 2023
* guard against null

* Create ArgumentNullTests.cs

* Update Guard.cs
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.

None yet

2 participants