search

lf-lang / lingua-franca

Intuitive concurrent programming in any language
https://www.lf-lang.org
Other
240 stars 63 forks source link

Add style check to build process #1052

Closed housengw closed 2 years ago

housengw commented 2 years ago

Seems like we could add spotless (https://github.com/diffplug/spotless) to our build process to perform automatic style checks. Works with Java, Kotlin and many other languages,

cmnrd commented 2 years ago

I think this is a great idea! However, more important than style checking in CI, is that we agree on a common workflow and style. Currently, it seems everyone uses different tooling and we have constant refactoring of code going back and force between different styles. Looks like spotless could also help solving this issue, as it integrates with various IDEs (but apparently not Eclipse...).

lhstrh commented 2 years ago

Agreed. This came up while we were working on a code style proposal, which we plan to share and discuss in the group next week.

petervdonovan commented 2 years ago

I just thought I would take the opportunity to share something neat that I found. This method in ASTUtils has 88 lines with a cyclomatic complexity of close to 20 (I think), which is about double the score that seems to be conventionally used as an upper limit.

If LF semantics were inherently that complicated, we would probably need to have simpler semantics. However, it seems more likely that we just need CI to automatically reject methods with a cyclomatic complexity greater than 10.

petervdonovan commented 2 years ago

Here is the output of a few formatters that integrate with Spotless. There are several other options, but these are perhaps the most similar to our current style. This is relevant to #478 because if we choose an automatic formatter, it might not be compatible with a customized set of project-specific formatting rules. For example, the only significant customization that is possible with googleJavaFormat is choosing between 2 and 4 spaces of indentation.

It would be great to be able to just pick an automatic formatter (or else close this and #478 and let everyone format files however they want) so that we can liberate humans from drudgery, reduce bikeshedding during code reviews etc., and avoid turning style into a years-long project

The Gradle configuration used to try out this formatting appears in #1374.

cmnrd commented 2 years ago

I absolutely agree to the above. We should setup a formatter and use it consistently across different developer setups and also enforce it in CI. I think its most important to just go for one of the formatters and less important for which. I don't have a strong preference to any of the presented formatters, but I have a few thoughts that I would like to share.

Personally, I really like the philosophy of formatters that are not configurable. It is either use it or not, and no time is wasted with discussions on the format configuration. That said, I believe however that clang-format also has several advantages. In particular, we could use it to create consistent styles in our Java code base and in the C and C++ runtimes. If we want this consistency, we might want to use the clang-format style that we already use for reactor-cpp. It is based on the LLVM style and can be found here. Also, here is the reactor-cpp workflow for checking the style in CI.

petervdonovan commented 2 years ago

Yes, I am also happy with any format we choose and have no strong preference. I would be 100% on board with just adopting clang-format for everything with LLVM style, if other people are also willing to do that. Some other comments, though, are that

I should also clarify that Spotless (which has a clang-format integration) has a "ratcheting" feature that automatically formats files that have been touched rather than formatting everything all at once. This could make the transition fairly smooth and low-effort.

cmnrd commented 2 years ago

If we use a formatter that automatically re-flows documentation comments, then I suspect a uniform line length of 120 characters will be unacceptable for those of us who have trouble following lines that are that long. We discussed this in Sonoma this summer and the consensus seemed to be against documentation comments that are 120 or even 100 characters. I agree with the "minimal discussion, minimal exceptions to the chosen style" idea, but this is different since it's an accessibility issue

Are there formatters that support separate rule for line width of code and comments? clang-format does not support this unfortunately, although I think it would be a useful feature. We can do the comment formatting manually though. clang-format will only touch comments that are longer than the column limit, but not if they are manually formatted to be shorter.

petervdonovan commented 2 years ago

Are there formatters that support separate rule for line width of code and comments?

I have had difficulty finding Java formatters that support such separate rules. I have argued unsuccessfully that 100 columns everywhere could be a good compromise (google-java-format, the black-like aggressive formatter that matches the Google style guide, does this), but we may have more success (at pleasing everyone by) using clang-format with 120 columns and manually re-wrapping.

cmnrd commented 2 years ago

Maybe we should make this a (short) discussion at the retreat. It would be great if we could try to reach a consensus and then just move forward with this.

lhstrh commented 2 years ago

I'm fine with 100, personally. I can even do 80. I've heard it being argued that screen resolution has increased and therefore screen real estate should no longer be considered a limiting factor for line width, but we also still prefer to print text on book pages instead of rolls of paper -- it's simply more ergonomic that way.

lhstrh commented 2 years ago

This has been addressed by @petervdonovan in #1374.