-
Notifications
You must be signed in to change notification settings - Fork 0
Add benchmark for source-generated SetBinding #25
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
Add benchmark for source-generated SetBinding #25
Conversation
|
Thank you for your pull request. We are auto-formatting your source code to follow our code guidelines. |
|
Thank you for your pull request. We are auto-formatting your source code to follow our code guidelines. |
| category: "Usage", | ||
| defaultSeverity: DiagnosticSeverity.Warning, | ||
| isEnabledByDefault: true), | ||
| defaultSeverity: DiagnosticSeverity.Hidden, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to keep some ""diagnostic"" even if we do not show it to the user. Currently it will be reported as hidden, we can introduce an explicit list of diagnostic we do not report and filter out before reporting ,if this is not enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this still be shown in the CLI output or in VS/VSCode?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried running the sample app with different overload, did not see any warnings/errors. When passing func it shows correct error.
| { | ||
| return new EquatableArray<DiagnosticInfo>([DiagnosticsFactory.SuboptimalSetBindingOverload(invocation.GetLocation())]); | ||
| } | ||
| var secondArgument = argumentList[1].Expression; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that we do the validation of the number of arguments in IsSetBindingMethod but it's still making me a little uneasy to just reach for an index without a check for the length of the array in the same scope. This could break if somebody refactors this code later. I'd just add ArgumentOutOfRangeException.ThrowIfLessThan(argumentList, 2);
|
Thank you for your pull request. We are auto-formatting your source code to follow our code guidelines. |
Uh oh!
There was an error while loading. Please reload this page.