Revisit code style checks

[mvglasow]

Currently my CircleCI builds keep failing because a code file is getting rejected for non-compliant style. Example: https://circleci.com/workflow-run/7ffa71d2-467e-4b51-a7a9-aa81d8053480

That would be the expected behavior if I had introduced these errors, but the file in question is a file I have not even touched in my branch.

Is there a way to limit style checks only to changes which are not yet in master (for branches), or since the last successful master build (for master)? Even if we could just do that at the file level (i.e. excluding files that have not been touched), that would already be an improvement.

[mvglasow]

I have just fixed a few files in a branch (every single one flagged by the sanity check), and after re-running CI, I am still getting new style errors that did not get flagged before. Something’s wring here…

[aerostitch]

I think #929 should fix your issue.

But I agree that there's something to do here and I was actually creating a GH issue about it so I guess I'll take this one and do the proposal here:

Hi guys,

Right now we have several tools to help make the code better (sanity check for non-java files and checkstyle for java files, codefactor, etc). The way we set them up has brought some frustrations in the contributions and I feel like we should focus more on getting working code in navit. What I'm proposing is not to get rid of what we did around the sanity check or codefactor etc but more to make it less mandatory.

I'm fine doing a bunch of PR to clean up some stuffs when I feel like it. I'm not fine with core contributors starting to be frustrated about the tools we put in place.

What I propose is:

• move the sanity check outside of circleci (especially not blocking the builds). It can be a github action for example, and make it not blocking.
• move the java checkstyle outside of the android build. Same it can be in a github action for example but making sure it's not blocking contributions
• revive coverity but make it not blocking

Once again, I'm fine having a PR merged with non-confirming style or pushing a code style fix in a PR of someone else before merging but I think the current way of doing things is bringing more frustration than benefits which was really not the original idea.

Any comments / issues / enhancements proposal around that change in the implementation?

Thanks Joseph

[metalstrolch]

From my point of view, the first thing to do would be to document the style requirements. Or better point to the documentation if it is already there. Personally I don't have any problems in e.g. running astyle, but having to inspect CI output to find the embedded Java style error or to find out what sanity_check does (for the non "C" coders) is not simple. Nobody likes that kind of surprises. Specially if you want (or have to) write code for architectures / languages you are not familiar with.

So we have: for C: astyle --indent=spaces=4 --style=attach -n --max-code-length=120 -xf -xh $(file) for Java checkstyle (don't kow the comand line) anything else?

Could we add checkstyle to sanity_check? Then it's failing at the same step, not somewhere inside the java part of the android build?

[aerostitch]

I started to document that in https://navit.readthedocs.io/en/trunk/development/programming_guidelines.html but good point I'll add more doc around that. I need to figure out how the java part works and see if we can move it to the sanity_check part.

[aerostitch]

So let me know if I get all the points right but the summary would be:

• See if we still end up with style checks on files you didn't touch now that #929 has been merged (Note that it could still be the case if your branch is not up-to-date but that's an easy fix just click on the "update branch" button to fix that)
• Update the PR to make it more obvious where to find the doc about the different checks
• Add the coding style for Java in https://navit.readthedocs.io/en/trunk/development/programming_guidelines.html (it's mostly documented in the checkstyle.xml but it'd be good to have it in the doc probably) and what to run to verify that locally (maybe come up with a multi-platforms git hook that could do that for you automatically?)
• Add checkstyle in sanity_check #936

Anything else or you think that should alleviate the frustrations you've had around that?

[metalstrolch]

In https://navit.readthedocs.io/en/trunk/development/programming_guidelines.html we should add about Java that it's quite stricter than "simple rules" and barely obey able without having checkstyle (and even gradle?) locally. Besides, does anybody know the minimum version of checkstyle that is compatible? The one featured with my Gentoo distribution doesn't understand some of the rules in the checkstyle xml file.

[metalstrolch]

While on it, please mention the presence and purpose of xmllint somewhere. That's one that is easily overseen, but luckily seldom touched. In former times we had that "graphics" CI inspecting the rendered images. Anything that like still around? I've read something about a routing QA?

[jandegr]

Hi, there is the QA https://github.com/navit-gps/routing-qa/pull/5

[lains]

Hello all,

Could we add checkstyle to sanity_check? Then it's failing at the same step, not somewhere inside the java part of the android build?

Yes, I agree on that because passing a first codestyle and getting further failures at subsequent steps, but on codestyle again can be quite frustrating or time consuming. In my opinion, it would be best to run all codestyle and sanity altogether even when there is a failure in one of them, so that we get the whole list of changes, and try to fix them all at once, thus saving time to wait for multiple subsequent new passes of codestyle.

I think automated check goes in the right direction by targeting a better code quality and maybe even catching potential errors. I think errors on codestyle should not stop compilation steps however. The reason why I'm saying this is because sometimes pushing a few commits is only about getting the result of the build process and continue working on a PR. Taking care of the style only really make sense when the code and architecture are stable. Maybe a Github action would be a good thing. Did I understand correctly that this would translate into having a "Go" button in the Github checks?

In any case, I think it is important to get the results of the codestyle and sanity before the PR is proposed for merge. Then, if passing codestyle and sanity become mandatory is another story, and maybe this should be left to the person that will perform the merge to trunk.

Thanks anyway for working on this topic, and thanks to all people that brought work and effort towards getting all these automated checks. In an ideal word, when a change is suggested by lint or codestyle, getting a script to automatically apply these suggested changes is what is needed. The machine proposes, the developer then selects and commits. I think getting a list of all checks that are run (depending on languages) and how to apply the changes automatically (whenever possible) is a very interesting step towards that.