Use [CallerArgumentExpression] for parameterName argument in Check methods #33524
Conversation
…methods - Remove explicit setting of parameterName from uses of these methods Fixes dotnet#33523
There was a problem hiding this comment.
Thanks - good code quality improvements, but see comments/suggestions below.
BTW we should also probably do this to Check.DebugAssert as well, making the 2nd argument optional; for places where we want a custom message, that can still of course be passed, but it's nice to not have to specify it and get the actual argument expression for free (/cc @ajcvickers)
src/Shared/Check.cs
Outdated
| { | ||
| if (value is null) | ||
| { | ||
| NotEmpty(parameterName, nameof(parameterName)); | ||
| NotEmpty(parameterName); |
There was a problem hiding this comment.
Unless I'm mistaken, with [CallerArgumentExpression] the argument can never be empty (i.e. it's guaranteed to be populated by the compiler)
There was a problem hiding this comment.
It's populated by the compiler if it's not explicitly set, but it can still be explicitly set, so this check may be worthwhile against future misuse. But I'm happy to remove it if you're sure.
There was a problem hiding this comment.
Right; though passing a null here would go against the nullable annotations for that parameter, so I don't think we need to check for that (or have a special check for the empty string...). So yeah, let's remove it.
There was a problem hiding this comment.
Removed all NotEmpty checks on parameterName
| @@ -10,7 +10,7 @@ public class CheckTest | |||
| [ConditionalFact] | |||
| public void Not_null_throws_when_arg_is_null() | |||
| // ReSharper disable once NotResolvedInText | |||
| => Assert.Throws<ArgumentNullException>(() => Check.NotNull<string>(null, "foo")); | |||
| => Assert.Throws<ArgumentNullException>(() => Check.NotNull<string>(null)); | |||
There was a problem hiding this comment.
nit: just for completeness, these tests could also check that the right message is thrown
There was a problem hiding this comment.
Added tests which check the right ParamName is supplied. Noticed the ParamName was not supplied for some, so fixed this. Note that HasNoNulls was using the parameterName as the message. Have not added tests for messages too but can if really needed. Using ConditionalTheory even for single test case so it is actually checking an argument.
|
@stevendarby just a reminder to request a new review (upper-right corner) when you're done with modifications. |
|
@stevendarby just a ping to make sure you haven't forgotten about this. |
|
@roji sorry for the delay, had some build issues last time I tried to address the comments- I'd just updated to the latest VS preview, not sure if related. Will have another go this evening and post back either way, but if you want to jump in before then please feel free! |
There was a problem hiding this comment.
LGTM, thanks @stevendarby. I went ahead and pushed a merge to resolve conflicts and will merge.
src/Shared/Check.cs
Outdated
| using JetBrains.Annotations; | ||
|
|
||
| namespace Microsoft.EntityFrameworkCore.Utilities; | ||
|
|
||
| [DebuggerStepThrough] | ||
| //[DebuggerStepThrough] |
There was a problem hiding this comment.
Am assuming this change was temporary and needs to reverted?
|
@stevendarby there are some minor-looking test failures, if you have time can you please take a quick look? If not please let me know. |
|
@stevendarby ping, let me know if you intend to continue working on this. |
|
Sorry @roji, I can't look at this any more |
Fixes #33523