Issue 458 WithParam: throw exception if parameter starts with '$' (or… #460
Conversation
… any other not allowed character). Issue 459 WithParam: exeption when using already existing param name outputs "key" instead of param name
There was a problem hiding this comment.
Hey Reiner,
Thank you for doing this, the exception messages need to be changed for the PR to be accepted, as setting the ParameterName property to being the nameof(theParameter) is important from the POV of what the actual exception should be passing around.
Otherwise, all good
| if (parameters.Keys.Any(key => QueryWriter.ContainsParameterWithKey(key))) | ||
| throw new ArgumentException("A parameter with the given key is already defined in the query.", nameof(parameters)); | ||
| throw new ArgumentException("A parameter with the given key is already defined in the query.", parameters.Keys.First(key => QueryWriter.ContainsParameterWithKey(key))); |
There was a problem hiding this comment.
This has the same issue as with the previous comment, the parameter called parameter is the problem.
The reason it should be this is if another parameter is added to the method, or something like that, you distinguish between those with that. The message can include the key that was invalid, but not the ParameterName property.
There was a problem hiding this comment.
Hi, same here, nameof(parameters) is always "parameters". But the value of parameters.Keys.First(key => QueryWriter.ContainsParameterWithKey(key))) contains the key that is used duplicate.
There was a problem hiding this comment.
I just added a test for this to check duplicates in WithParams in the PR
There was a problem hiding this comment.
And my argument still stands, the fact is that the parameter to the method is wrong, if the method had 4 parameters, all string (for example) and you just fire out the 'value' of the one that is wrong, you wouldn't be able to tell which of those parameters was wrong.
Value can go into the message, but the parameter name that is wrong is parameters
| if (parameters.Keys.Any(key => QueryWriter.ContainsParameterWithKey(key))) | ||
| throw new ArgumentException("A parameter with the given key is already defined in the query.", nameof(parameters)); | ||
| throw new ArgumentException("A parameter with the given key is already defined in the query.", parameters.Keys.First(key => QueryWriter.ContainsParameterWithKey(key))); |
There was a problem hiding this comment.
And my argument still stands, the fact is that the parameter to the method is wrong, if the method had 4 parameters, all string (for example) and you just fire out the 'value' of the one that is wrong, you wouldn't be able to tell which of those parameters was wrong.
Value can go into the message, but the parameter name that is wrong is parameters
|
Changes pushed - sorry for the long-winded discussion. |
… any other not allowed character).
Issue 458 WithParam: throw exception if parameter starts with '$' (or any other not allowed character).
Issue 459 WithParam: exeption when using already existing param name outputs "key" instead of param name