-
Notifications
You must be signed in to change notification settings - Fork 31
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
Create rule S6673: Log message template placeholders should be in the right order #2563
Conversation
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.
Proposed some restructuring and some rewording, for better clarity.
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.
The C# version looks good to me.
I left a couple of minor comments.
Requesting changing to add the VB.NET part.
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.
Some minor issues with file naming and structure, and diff-ids.
In this scenario, I would have probably tried writing code valid for both C# and VB.NET (not using named argument, semicolon, and moving comments to normal text before the examples), to avoid all these example_n_compliant/noncompliant.adoc
files.
But that's a personal choice, and I find very accurate and structured your approach.
SonarQube Quality Gate for 'rspec-tools' |
SonarQube Quality Gate for 'rspec-frontend' |
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.
LGTM!
e42421e
to
b7134f8
Compare
b7134f8
to
8335972
Compare
|
|
You can preview this rule here (updated a few minutes after each push).
Trello card
Review
A dedicated reviewer checked the rule description successfully for: