ToughTechs151 / robot-template

Template for Tough Techs robots.
Other
1 stars 0 forks source link

Make sonarlint and checkstyle accept the m_ style of method fields. #27

Closed blu28 closed 1 year ago

blu28 commented 1 year ago

WPI uses the m style in their code, but Google has forbidden it in their style. Both checkstyle and Sonarlint use the Google style. In order to make import of WPI examples easier, we should not flag m as naming violations. Ideally we should be able to make it easy to toggle this back and forth.

blu28 commented 1 year ago

Here are some things I learned:

The format for fields is checked in both Sonarlint and Checkstyle. The format for Constants is only checked in Sonarlint. Sonarlint rules can be modified in the User settings.json file, but not in the workspace settings.json file. The rules for checkstyle are read from a file which can be checked into the project and the info directing checkstyle where to find the file can be placed in the workspace settings.json file. I can modify the regex in the checkstyle file to allow the m_ prefix. Since checkstyle doesn't enforce the constants format, checkstyle is out of the picture.

We can change the regex for Sonarlint, but since it is in the User settings.json file, it would have to be modified by hand by the user, which is error prone and also means it would apply to every project the user works on. There would be no way to be permissive in one and strict in another.

It is possible to have project level Sonar rules by using SonarQube instead of SonarLint. SonarQube check more things as well, so that may be a good way to go, but the SonarQube project has to be configured at the very least (another manual step) and may need to be configured by each user, which would be even worse. Still it might be something to explore.

A better approach would be to use the @SuppressWarning annotation directive to tell SonarLint to ignore the error for specific fields and constants. But the user needs to know the format of the annotation, and the rule number they want to suppress. Getting the rule number is easy on a case by case basis, simply right click on the problem listing and it tells you. Since you can combine multiple rules to suppress in a single annotation, it might be simpler to always include all three rules (constants, fields and commented out code) in the suppresswarnings:

@SuppressWarnings({"java:S115", "java:S116", "java:S125"})

But that is a lot to remember. I put this command in the snippets file, so all the student will need to do is put the cursor at the beginning of the line, type ctrl-space and then "so" (for sonarlint) and it will insert the annotation. However, I think we can make it even easier. Ideally there would be a keyboard shortcut or command in the command palette that would toggle this line when the cursor is anywhere on the line where the variable is defined. Unfortunately there is no way to insert a snippet on a previous line, they only get inserted where the cursor is. Also, type the beginning of the snippet in the middle of a word screws up the snippet lookup and the word the cursor is on.

However, there is an extension called "multi-command" which lets you run editor commands like a script, so it should be possible to insert the snippet on the previous line. With conditionals it should also be possible to delete an existing Suppress line. Which may lead to being able to do a toggle.

This extension also gives a lot of other possible uses, such as toggling the severity of the spell checking errors, among others. This looks like the way to go. I have added the extension to the VSCode.md file.

blu28 commented 1 year ago

It turns out that I already set up an Organization account on SonarCloud for Tough Techs last year, so that part is done. Adding a project to SonarCloud is trivial and free if the repo is public, so no problem there. To connect a given VSCode instance to the cloud takes an individual account and some configuration. I am not sure that is worth it. That is not required to get the analysis, it can be done at the repo level. That might be the better solution, get the analysis from the repo rather than in VSCode.

blu28 commented 1 year ago

Unfortunately VSCode does not let you set up keybindings on a workspace basis, so using multi-command is of limited utility for this issue, but it is something to keep in mind for the future, it could be useful.

It looks like the best way to go is to use a predefined checkstyle file that has been modified to allow "m_" at the beginning of field names, so it will accept WPI style. There is no check in Checkstyle for Constants being capitalized, so both of those will be detected in Sonarlint, but not Checkstyle after the change. Another common naming problem is that Checkstyle doesn't like consecutive capitalized letters. We can add a list of exceptions, so we can use "PDP" and "PID", etc. Sonarlint doesn't seem to have a rule about abbreviations.

I added a snippet called "slname" so to suppress the naming Sonarlint warnings, all you have to do is start a new line on the line before the error, and start typing "slname" and auto-complete will pop it up. One more thing, putting the suppress line at the beginning of the class suppresses the warning for the entire class. I think this is an acceptable solution.