matsim-org / matsim-libs

Multi-Agent Transport Simulation
www.matsim.org
490 stars 452 forks source link

Should we put a editorconfig or similar into the project #1333

Open Janekdererste opened 3 years ago

Janekdererste commented 3 years ago

I use autoformatting and clean up of imports and such things in IntelliJ, because I don't want to bother with formatting my code. I also am too lazy to change any of the default settings of my idea, because I don't really care how the code is formatted. Now, this leads to situations where my idea removes spaces in a case like the following

someMethod( someParameter );

because it thinks the spaces around the parameter don't belong there. If another person then edits this file the spaces are put in again because the other idea thinks spaces are important. This leads to a lot of edits and makes finding the actual changes in commits dificult.

Should we place a code style file in the repository to make importing common formatting rules easy? Options are editorconfig, IntelliJ-style xml, Eclipse style xml, I think.

In the JavaScript world opinionated formatters like Prettier are often used. This prevents discussions about the right style. Maybe there is an option like this for Java as well?

I guess @kainagel @mrieser @michalmac @rakow @kt86 @vsp-gleich @jfbischoff might have an opinion on this.

michalmac commented 3 years ago

We started a similar discussion some time ago: https://matsim.atlassian.net/browse/MATSIM-918.

In the meantime I added .editorconfig to a few dvrp-related modules. Seems to work, but it does not bring us close to having a unified formatting, which would greatly improve coding experiences.

rakow commented 3 years ago

I always tend to add a .editorconfig to my repos. The example of @Janekdererste is not covered by the standard, though.

You can set them for Intelij anyway (https://blog.jetbrains.com/idea/2019/06/managing-code-style-on-a-directory-level-with-editorconfig/), but I don't know about other editors.

michalmac commented 3 years ago

In the jira thread, one proposed option was to use google-java-format. It's really well maintained and widely supported (IDEs, editors...). But its formatting is so much different to ours, so that would be a huge change.

So maybe in our case having a MATSim-customised Eclipse formatting config is a better option? There is some support for mvn and IntelliJ:

kainagel commented 3 years ago

If those people who participate at this discussion here can agree on a solution, I will be ok with it.

My wishlist would be as follows:

(1) indentation by tabs (since then I can choose myself if I want wide or narrow indents)

(2) I am not a fan of (maybe not so important (but often difficult to see (and sometimes weird))) brackets opening or closing without space in between.

However, if these two are the only two obstacles to a common agreement, they can be ignored.

One thing that is not a wish but a requirement: If we do not make it automatic through the build server, it will not work. E.g. it should fail the build if not correctly formatted, or maybe reformat on the fly.

vsp-gleich commented 3 years ago

We have some commits which are really hard to understand because the changed lines are 95% automatic code reformatting with 5% real changes which are hard to find. So I think agreeing on automatic code formatting is a really good idea. I have no clear preference for a specific style, but I think it's good to keep the line length rather long (e.g. 130 characters), because I actually find that easier to read with our long class and variable names. I also think having a space at the opening and closing of brackets is nice and that indentation by tabs has sense.

Maybe we should combine the introduction of auto-formatting with a single commit that auto-formats the whole repository? So from then on we only have "real" code changes and no lines changed only due to formatting.

One thing that is not a wish but a requirement: If we do not make it automatic through the build server, it will not work. E.g. it should fail the build if not correctly formatted, or maybe reformat on the fly.

I agree with Kai here.

kt86 commented 3 years ago

I totally agree that we should have it and I am totally agree with @kainagel and @vsp-gleich. I also have no clear preference for the concrete style. Longer lines, indentation by tabs and spaces at opening and closing of brackets makes also sense to me. And I also like the idea of a one-time reformatting the whole repository.

This topic is also important because some people are using (different) auto-formatting styles, so it get reformatted every time from style A to B and vice versa. --> This is why I (re)started discussing it with @Janekdererste

michalmac commented 3 years ago

One thing that is not a wish but a requirement: If we do not make it automatic through the build server, it will not work. E.g. it should fail the build if not correctly formatted, or maybe reformat on the fly.

I agree with Kai here.

That's the idea behind the mvn autoformat plugins: