search

wpilibsuite / allwpilib

Official Repository of WPILibJ and WPILibC
https://wpilib.org/
Other
1.04k stars 606 forks source link

Add enforcement of wpilib's java style when using vscode. #6669

Open brettle opened 1 month ago

brettle commented 1 month ago

This will automatically format modified lines (according to wpilib's java style) when a java file is saved in vscode.

This requires including .vscode/settings.json in the repo which appears to be a fairly widely accepted practice in large projects (including vscode itself and mozilla). For details, see: https://stackoverflow.com/questions/32964920/should-i-commit-the-vscode-folder-to-source-control

ThadHouse commented 1 month ago

Theres actually a long standing VS Code issue about workspace local settings

https://github.com/microsoft/vscode/issues/40233

Until that happens, I wouldn't want something like this, as I run a very different settings.json.

brettle commented 1 month ago

I wouldn't want something like this, as I run a very different settings.json.

Can your user-specific settings be put in your user settings instead of the folder settings? If not, would adding them to a shared setttings.json actually break things for others?

ThadHouse commented 1 month ago

I'll have to double check. But probably would be opposed to format on save either way. I've seen too many times where it doesn't always get things right, and then you have to remember how to save without formatting.

brettle commented 1 month ago

I could live without the format-on-save being in the folder settings. I could move that to my user settings. I am primarily interested in getting the wpilib specific style in the folder settings. Would that work for you?

ThadHouse commented 1 month ago

One of the biggest things I worry about is when you're working on the C++ side, a lot of random file associations get added, in the files.associations key. So there always ends up being a lot of changes to that file. Thats probably an issue in the C++ extension, but its still something that has me worried.

It's really easy to add this locally, I'm not fully convinced adding it to the repo is worth it. We could document how to get settings like this working.

sciencewhiz commented 1 month ago

One other concern is that I don't think this will fully cover all the formatting that CI enforces. I find I trigger spotless a lot more often then wpiformat.

brettle commented 1 month ago

One of the biggest things I worry about is when you're working on the C++ side, a lot of random file associations get added, in the files.associations key. So there always ends up being a lot of changes to that file. Thats probably an issue in the C++ extension, but its still something that has me worried.

Does my most recent commit address that worry?

We could document how to get settings like this working.

I'm not yet convinced that it can't be made to "just work". :smile:

brettle commented 1 month ago

One other concern is that I don't think this will fully cover all the formatting that CI enforces. I find I trigger spotless a lot more often then wpiformat.

Fwiw, I started looking into this because of a formatting issue that wpiformat didn't catch. At least for that particular issue, my testing indicates that this would have fixed it. And even if it doesn't catch all issues, it would still be a step in the right direction.

ThadHouse commented 1 month ago

Does my most recent commit address that worry?

No, because that doesn't work on Windows.

calcmogul commented 1 month ago

Fwiw, I started looking into this because of a formatting issue that wpiformat didn't catch. At least for that particular issue, my testing indicates that this would have fixed it. And even if it doesn't catch all issues, it would still be a step in the right direction.

That's because wpiformat is mainly for formatting C++ files, and spotless is for formatting Java files. The only formatting wpiformat does to Java files is remove trailing whitespace and blank lines after open curly braces.

The issue you had in that PR would have been fixed by spotless's javadoc comment reflowing via ./gradlew spotlessApply.

brettle commented 1 month ago

that doesn't work on Windows.

Works for me on Windows. Did you try it? Note that the glob will match the headers installed by the wpilib installer at "*/2024/roborio/arm-nilrt-linux-gnueabi/sysroot/usr/include/c++/12/*". If you need it to match system headers installed elsewhere, I'm happy to try to create an additional pattern to match them if you let me know where yours are.

ThadHouse commented 1 month ago

that doesn't work on Windows.

Works for me on Windows. Did you try it? Note that the glob will match the headers installed by the wpilib installer at "*/2024/roborio/arm-nilrt-linux-gnueabi/sysroot/usr/include/c++/12/*". If you need it to match system headers installed elsewhere, I'm happy to try to create an additional pattern to match them if you let me know where yours are.

It needs to match the MSVC headers too, which are all over the place.

brettle commented 1 month ago

wpiformat is mainly for formatting C++ files, and spotless is for formatting Java files. The only formatting wpiformat does to Java files is remove trailing whitespace and blank lines after open curly braces.

Good to know but not at all clear to new contributors.

The issue you had in that PR would have been fixed by spotless's javadoc comment reflowing via ./gradlew spotlessApply.

...or, if this PR was accepted, it wouldn't be an issue to begin with. :smile:

I do appreciate the tip though. Thanks!

brettle commented 1 month ago

It needs to match the MSVC headers too, which are all over the place.

Does my latest commit match them?

spacey-sooty commented 1 month ago

I don't like this change, if people want to configure auto formatting for their editor they can it's not the responsibility of the project. I'll also repeat C++ file association concerns other people have brought up, this could be annoying to see popping up occasionally in prs.

brettle commented 1 month ago

Ok. My latest commit takes a (hopefully) less controversial/intrusive approach. It makes the settings optional but recommended. It does so by putting the relevant settings in settings.default.json instead of settings.json and having vscode recommend an extension that will, by default, automatically merge settings from settings.default.json into settings.json. The default settings are designed to be non-controversial, but users who object to them can silence the recommendation of extensions (via the extensions.ignoreRecommendations setting) and/or prevent the recommended extension from automatically merging settings (via the workspace-default-settings.runOnActivation setting). Note that the merging does not overwrite the whole file; it only overwrites individual entries in settings.json with the corresponding entries from settings.default.json.

Does that address everyone's concerns?