Fix for issue 19: not recognising params object[] in custom abstraction #20
Conversation
which just checks if the given parameter is "params", and in that case adds the argument to be checked, as well.
to check if it is of type object.
There was a problem hiding this comment.
I think going with just parameter.IsParams would be fine. params requires an array type (currently, i think i've seen a proposal to allow IEnumerable as well) but the point is, that params allows a variable amount of parameters to be supplied so i don't think we should enforce a specific type, maybe someone wants to require params string[] values for some reason.
| { | ||
| int id = 1; | ||
| IMyLogger log; | ||
| log.Error(""The id is {Id}"", id); |
There was a problem hiding this comment.
log is being used uninitialized, should be at least null initialized
There was a problem hiding this comment.
Thanks, I'll fix that.
| var location = argument.GetLocation().SourceSpan; | ||
| arguments.Add(new SourceArgument { StartIndex = location.Start, Length = location.Length }); | ||
| } | ||
| else if (parameter.IsParams && IsObjectArray(parameter)) |
There was a problem hiding this comment.
The bodies seem identical to me, should just be an || condition?
There was a problem hiding this comment.
I'll fix this, if we don't go with the less strict approach (which would fix that anyhow).
|
Further thinking about the issue, my original intent behind this part of the code was to prevent confusing message template arguments with any other argument. Unfortunately, the An alternative approach would be to use anything following the For example, if you would be to implement new methods that prevent the array allocation in case the |
|
Not sure if this helps, but when a message template is used, at least in all of the current libraries, it'd be fair to assume that any arguments to the right of it are either a In Serilog, and hopefully elsewhere, "special" arguments like |
|
When I was working on the issue, I was already in doubt if a strict approach makes sense. Also hearing Nicholas' feedback, I'm quite certain that going with "everything after the @Suchiman Do you agree? Shall I'll update the PR with that? |
|
@tiesmaster Yes, that's fine with me |
fixes compiler error on analyzer input test code.
by treating all parameters after the template message parameter as arguments for the template, which is the most widely used approach people use for method signatures with message templates.
|
@Suchiman I've updated the PR, as discussed. Can you have another look at it? |
|
Looking good! |
As discussed a PR to fix #19 for me. This recognises
params object[]parameters in a call to the logger, and takes that into account.A couple things to note:
I put in an unit test for specifically reproducing this scenario. I first tried to reproduce it on the Serilog logger, however, I found out that it doesn't fail there, since the
params object[]also starts with "propertyValue" (so it actually works correctly for Serilog!). This way I was able to reproduce it for me.I thought there was an easy check, or extension method for checking on
params object[]parameters, but I couldn't find it. I did find this SO answer for verifying the type, but I'm not 100% sure if this is the right approach. We can also just go withparameter.IsParams && parameter.Type.Kind == SymbolKind.ArrayType, or even justparameter.IsParams.