Move checkstyle into the sanity_check phase

[aerostitch]

Trying to work on #725

[jandegr]

Hi,
You're mixing things up here with .... ?
the opening post in 725 is about asyle.

[aerostitch]

Hi @jandegr ,

I'm not sure I understand your remark so feel free to correct me.

In the thread in 725 (which has expanded to more than astyle) there is one request (if I understand it correctly) about having both the C/C++ (astyle) and Java (checkstyle) code style checks done at the same level.
The idea behind that is to avoid having to wait for a builder to start the android build before discovering that your PR has style issue when doing java code changes.

One of the issues that got people discouraged by waiting for java style check was #889 for example.

Does that makes sense?

Thanks
Joseph

[jandegr]

Hi,

Instead of 'expanding' in 725, the original issue is just dropped and 725 is just hijacked for other purposes, and we all know what those are.

[aerostitch]

I'm not sure I understand what you mean. The title was"Revisit code style checks" and raised one of the problems that was faced to illustrate one of the frustrations. As I said in that issue I think the astyle issue has been fixed in another PR. Now if you're saying that there's some hidden evil purposes behind the expansion of that I can assure you there's none from my side.

Now if there's a problem in 725 maybe we want to treat it there and evaluate here if the proposed change is something that makes sense and that we want. If you feel strong about this change not being the right way to address the wait for a builder to start the android build before discovering that your PR has style issue when doing java code changes issue, we can close it or change it and try to find a better approach that would maybe alleviate the frustration that has been happening.

[jandegr]

ah whatever, you protested against checkstyle last year and now I brought it to zero virgule zero issues and blocking without your permission, so sure that will be changed. I am the very last one who will oppose so no need to beat around the bush.

[pgrandin]

I'm not sure why people are getting twitchy. We're here to work towards the same goal.

@jandegr :

the original issue is just dropped and 725 is just hijacked

are you referring to the issue being dropped in the discussion or in this pull request? From what I read the intent of this pull request isn't to fix everything that was listed in the issue, but to start working on it. This looks fine to me.

you protested against checkstyle last year

actually I think that @aerostitch is the one who implemented it

Now, our current checkstyle/sanity checks is far from perfect. Yesterday I pushed a translation update and it complained about a .c file that I didn't touch. Annoying, but we just need to fix these glitches and soon we'll have a pipeline that will help us enforce and uphold better coding practices and standards.

[aerostitch]

actually I think that @aerostitch is the one who implemented it

No. @jandegr did the implementation and did a great job at it.

ah whatever, you protested against checkstyle last year and now I brought it to zero virgule zero issues and blocking without your permission, so sure that will be changed. I am the very last one who will oppose so no need to beat around the bush.

@jandegr If you're referring to the heated discussion of last year where I took with less than empathy that I should have the fact that the merge of astyle messed up the work you were doing on checkstyle I'm really sorry. I don't think I properly apologized for that so here it is: my sincere apologies for that.
I've been through pretty tough times over the last 2 years and sometimes behaved like a real dick.

To be honest I think the work you did on checkstyle is really great and I don't want to put it in question, just move it more forward in the build process.

I know that this last year discussion ended up with you leaving the org and I think I speak for everybody when I say that we'd really like to have you back in the org full time.

You bring way more to the org than I do and if it would help I can stick to the docs migration and not touch the rest. Just let me know.
If you would feel more comfortable with me out of the org doing only external contributions I'm fine with that too. Just let me know.
But really here I'm just trying to make everybody's life easier around Navit.

Note: this PR is really not to hijack your work. It's just to put the check of both C and Java on the 1st step to get the detection of issues quicker.

[jandegr]

hi,

apologies for that

thx, but really no need to,really not, was partially caused by bad timing.

then for the content itself, they just will come up with any kind of excuse to make it not-blocking anymore as you aready set as a goal in 725, not blocking
I knew this was going to happen beforehand, I don't even care a bit, and all the rest will be happy if the blocking status is removed. Once that is done they are not at all interested anymore in when or where what runs, everybody happy.
You know the short track, just change the severity tags so it does not block anymore. Codefactor will keep reporting new issues, so no other checkstyle related work to do.

[aerostitch]

Hi,

I was proposing to make them not blocking because I was not seeing any other mean to remove the frustration around it but it seems that it's not going to be necessary based on the feedback which is great! It seems mostly to be more about keeping all the style checking (no matter the language) at the same place and enhance documentation, so that's somewhat positive :)

I've been working on cleaning up Codefactor when time permitted (and I personally don't mind doing that) but in a lot of PR I've seen people fixing issues shown by Codefactor in the code they were submitting even if it's only optional.

Open source is really things we do on our free time so it has to stay fun. That's why I've been worrying about the fact that some PR were abandoned because of frustration on how things work right now and I'm trying to find a good compromise to keep it appealing.

Hope that makes sense.
Don't be discouraged about all this. It's really about doing some minor adjustments to make contributing to Navit better. We've all cleaned a lot of stuffs over the last year thanks to the things we've been putting in place. I remember Codefactor showing over 1200 issues and now it's less than 500. Checkstyle is squeaky clean. astyle still seems to have hiccups but that's something I'll try to focus on if it still happens now that #929 is merged.

Once we've moved the rest of the wiki to readthedocs and been able to figure out what to do with trac I'll try to focus on continuing to cleanup our compilation warnings, then CodeFactor and after that Coverity (or maybe Coverity before CodeFactor we'll see). But don't worry, it's something that I think is important and is not going to be forgotten anytime soon.

[metalstrolch]

Approve this. Good Work.
Besides, I think we could keep those style checks mandatory, given there is a good to find documentation on how to obey them.
Alas I wouldn't make them block the rest of the build, and I'd make checkstyle a little bit more forgiving. The rules about spacing around language elements like if or "::" are too silly, aren't they? "Astyle alike" is enough for me.

[aerostitch]

Thanks for the approval.
I'll wait until navit-gps/Dockerfiles#9 is merged to use the corresponding image so that we don't add too many concerns to #910

[aerostitch]

I actually pushed the image and tested it, works fine so I'll merge as I updated the circleci to use the new image.

[jandegr]

May I ask if checkstylemain.html is dumped or is it just coincidence, for the record only.

https://15831-30791823-gh.circle-artifacts.com/0/reports/checkstyleMain.html

[aerostitch]

That's a great catch, thanks!
I fixed it as part of #941 as I'm modifying the .circleci/config.yaml in there. See 5306d79

[jandegr]

thx, and to keep my reputation, here's another one
Why install gradle ?

[aerostitch]

Sorry for the late reply I've bee busy.
What do you mean install gradle? In the image? I just download the zip so that it doesn't have to do it everytime (using the link defined in the wrapper: https://github.com/navit-gps/navit/blob/trunk/gradle/wrapper/gradle-wrapper.properties#L5)