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
Modify rule S4545: Add VB.NET specifics, Move C# specifics #1571
Modify rule S4545: Add VB.NET specifics, Move C# specifics #1571
Conversation
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.
Some minor comments. I'm not a fan of the name rule-expect-see.adoc, but I've seen it used in other rules as well, so it's just my preference.
0c34ec6
to
da704cd
Compare
I have restructured a bit the content:
|
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
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.
Generally looks good - only a minor change to the description to make it IMO clearer.
rules/S4545/csharp/rule.adoc
Outdated
The ``DebuggerDisplayAttribute`` is used to determine how an object is displayed in the debugger window. | ||
|
||
|
||
The ``DebuggerDisplayAttribute`` constructor takes a single mandatory argument: the string to be displayed in the value column for instances of the type. Any text within curly braces is evaluated as the name of a member, or any complex expression converted to a string value, containing method calls and operators. |
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 ``DebuggerDisplayAttribute`` constructor takes a single mandatory argument: the string to be displayed in the value column for instances of the type. Any text within curly braces is evaluated as the name of a member, or any complex expression converted to a string value, containing method calls and operators. | |
The `DebuggerDisplayAttribute` constructor takes a single mandatory argument: the string to be displayed in the value column for instances of the type. Any text within curly braces is evaluated as the name of a field, property, or method or as a complex expression containing method calls and operators that is evaluated to a string. |
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.
- Double backticks to single backticks => OK
- Member to "field, property or method" => OK ("method" removed, see here)
- "converted to a string value" to "That is evaluated to a string" => The second seems incorrect to me since expressions such as
DebuggerDisplay("{42}")
are correctly evaluated, despite42
not being evaluated to a string. It's rather that the expression is evaluated to whateverobject
it evaluates to, and then aToString()
is called on that object. In any case, I have removed the subordinate altogether.
rules/S4545/vbnet/rule.adoc
Outdated
The ``DebuggerDisplayAttribute`` is used to determine how an object is displayed in the debugger window. | ||
|
||
|
||
The ``DebuggerDisplayAttribute`` constructor takes a single mandatory argument: the string to be displayed in the value column for instances of the type. Any text within curly braces is evaluated as the name of member, or any complex expression converted to a string value, containing method calls and operators. |
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.
same proposal as above.
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.
Fixed in the same way it is described in #1571 (comment)
da704cd
to
1cf3bad
Compare
SonarQube Quality Gate for 'rspec-frontend' |
SonarQube Quality Gate for 'rspec-tools' |
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
Implementation SonarSource/sonar-dotnet#6703
Kept as draft as it is to be merged after SonarSource/sonar-dotnet#6728.
Remark: implementation merged before corresponding RSPEC is ready. Changes to the sonar-dotnet repo are to be expected, when running rspec update upon release.