Create rule S6934: A Route attribute should be added to the controller when a route template is specified at the action level #3676
Conversation
mary-georgiou-sonarsource
changed the title
mary-georgiou-sonarsource
changed the title
mary-georgiou-sonarsource
force-pushed
the
rule/add-RSPEC-S6934
branch
3 times, most recently
from
f0c3861 to
abf4c94
Compare
mary-georgiou-sonarsource
force-pushed
the
rule/add-RSPEC-S6934
branch
from
54e2854 to
d28c5ae
Compare
|
|
rules/S6934/csharp/rule.adoc
Outdated
| [source,csharp] | ||
| ---- | ||
| public class PersonController: Controller |
There was a problem hiding this comment.
The entire code is indented by 4 chars, which makes the code less readable on small screens or large zoom.
I would remove the 4 chars of indentation, and have it coherent with most of the other C# and VB.NET examples in the RSPEC repo.
There was a problem hiding this comment.
The indent of the signature has been reduced by one, but not the body, which was indented by 8 chars.
I have reduced the indentation of the body too.
rules/S6934/csharp/rule.adoc
Outdated
| //=== Conference presentations | ||
| //=== Standards | ||
| //=== External coding guidelines | ||
| //=== Benchmarks |
There was a problem hiding this comment.
We should add the rspecator.adoc, indicating:
- what we plan to highlight in the code
- what we plan to issue as message
There was a problem hiding this comment.
I have updated the message, taking into account the changes done at the implementation level.
There was a problem hiding this comment.
I have also updated the highlight for secondary locations, as it was inconsistent with the implementation, which doesn't highlight the route template of the controller action, but the identifier of the controller action.
|
@mary-georgiou-sonarsource I would start the review from this comment, as it may change quite a bit the content of the RSPEC. |
mary-georgiou-sonarsource
left a comment
mary-georgiou-sonarsource
left a comment
There was a problem hiding this comment.
Looks good - some minor comments.
mary-georgiou-sonarsource
assigned
martin-strecker-sonarsource and unassigned
mary-georgiou-sonarsource
Co-authored-by: Mary Georgiou <[email protected]>
| // C# | ||
| * ASP.NET | ||
| * ASP.NET Core | ||
| * ASP.NET MVC 4.x |
There was a problem hiding this comment.
According to https://dotnet.microsoft.com/en-us/platform/support/policy/aspnet asp.net mvc 4.x is no longer supported.
Are we sure we want to support it?
There was a problem hiding this comment.
That's true I think there has been maybe some confusion.
@antonioaversa @martin-strecker-sonarsource maybe we should review the two rules we merged already?
There was a problem hiding this comment.
The 4.x in "Asp.MVC 4.x" refers to the 4 in e.g. .Net Framework 4.8. The term is taken from this website:
https://learn.microsoft.com/en-us/aspnet/core/
It links to the documentation for the ASP MVC releases for the old .Net Framework 4.8. This includes ASP.NET MVC 5 which is still in support. So by referring to "Asp.MVC 4.x " we refer to "ASP.NET MVC 5"
I know this is highly confusing, but something we discussed with Denis and concluded is the best solution.
mary-georgiou-sonarsource
left a comment
mary-georgiou-sonarsource
left a comment
There was a problem hiding this comment.
LGTM - We need to resolve this question https://github.com/SonarSource/rspec/pull/3676/files#r1533861482
antonioaversa
left a comment
antonioaversa
left a comment
There was a problem hiding this comment.
Looks good to me too!
I have resolved most of the threads that I have opened myself and made minor modifications for the remaining.
|
|
antonioaversa
changed the title
4 participants
@antonioaversa
Implementation PR SonarSource/sonar-dotnet#8954