Title add script for running uncrustify
Priority feature Status resolved
Superseder Nosy List florian, jendrik, malte
Assigned To jendrik Keywords
Optional summary

Created on 2020-07-01.11:07:03 by jendrik, last changed by jendrik.

msg9428 (view) Author: jendrik Date: 2020-07-03.09:53:42

BTW: I have set up my editor to run uncrustify on every file change and I'm not looking back :-)
msg9423 (view) Author: malte Date: 2020-07-02.19:41:52
Also fine with me.
msg9415 (view) Author: florian Date: 2020-07-02.17:58:51
Fine with me. Like Malte, I'll miss the diff option and the option to run only on a specific file but I can live without it.
msg9414 (view) Author: jendrik Date: 2020-07-02.17:52:45
1) I agree that a "force" option could be useful. I added it to the patch.

2) Using the script directly produces less output.

Can I merge this?
msg9408 (view) Author: florian Date: 2020-07-02.17:04:34
I'm done with my review and only had a short comment on the code.

I also tried the process on git and it worked there. There are two things that bug me a bit, but I could live with both of them:

1) When I have uncommitted changes, I can run the style checks to see that there are problems but to see what the problems actually are, I have to commit the faulty version first. It seems like a strange incentive for people to check in wrongly formatted code. A "diff" option would be nice here, and maybe also a "force" option to do the modifications even if the changes are not committed.

2) The actual problem is a bit hidden in the output if there are style issues.
For example, this is the output if there is a problem with just one file:

style installed: appdirs==1.4.3,CacheControl==0.12.6,certifi==2019.11.28,chardet==3.0.4,colorama==0.4.3,contextlib2==0.6.0,distlib==0.3.0,distro==1.4.0,flake8==3.8.3,html5lib==1.0.1,idna==2.8,ipaddr==2.2.0,lockfile==0.12.2,mccabe==0.6.1,msgpack==0.6.2,packaging==20.3,pep517==0.8.2,progress==1.5,pycodestyle==2.6.0,pyflakes==2.2.0,pyparsing==2.4.6,pytoml==0.1.21,requests==2.22.0,retrying==1.3.3,six==1.14.0,urllib3==1.25.8,webencodings==0.5.1
style run-test-pre: PYTHONHASHSEED='1660022466'
style run-test: commands[0] | python
Running check_cc_files
Checking style of 210 *.cc files
Running check_cplusplus_style
Checking 432 files with uncrustify.
FAIL: /home/pommeren/Desktop/convert/converted/jendrik-downward/src/search/command_line.h (File size changed from 559 to 557)
Run "tox -e fix-style" in the misc/ directory to fix the C++ style.
Running check_include_guard_convention
Running check_python_style
Style checks failed
ERROR: InvocationError for command /home/pommeren/Desktop/convert/converted/jendrik-downward/misc/.tox/style/bin/python (exited with code 1)
_____________________________________________________________________________________________________________________________ summary ______________________________________________________________________________________________________________________________
ERROR:   style: commands failed

The last two ERROR lines are red so they draw focus but the actual important information is in line 9/16 which is not red. Likewise, if there are uncommitted changes, I get 9 lines of output (two of them in red) but the actual information is somewhere in the middle.
msg9404 (view) Author: malte Date: 2020-07-02.16:31:15
I had a quick look, not a real review, but from my side I don't need to review this again. If it works, it works.
msg9402 (view) Author: jendrik Date: 2020-07-02.15:13:50
New pull request:
msg9397 (view) Author: jendrik Date: 2020-07-02.00:59:54
Florian, if the bitbucket repo is gone before you get a chance to look at the patch, you can find the code at ssh://ai-repos/seipp/projects/downward .
msg9386 (view) Author: jendrik Date: 2020-07-01.19:11:27
Thanks for the tips, Malte! I updated the patch accordingly.

Florian, are you also happy with the patch?
msg9381 (view) Author: malte Date: 2020-07-01.16:07:26
I'm not sure I understand what exactly you mean, but I had a look and left some comments. The proof is in the eating, though. If it works for git and hg for you and Florian, fine to merge.

I liked the --diff option and used it frequently, but I'll survive if it's gone.
msg9378 (view) Author: jendrik Date: 2020-07-01.12:39:26
I made a pull request at 

It would be great if we could merge this today, so I don't have to "redo" everything in git. The new script allows for checking and modifying files but doesn't provide a diff of changes. Given that the script will only modify a clean repo, I don't think a diff option is necessary.

Malte, are you fine with the patch?
msg9372 (view) Author: florian Date: 2020-07-01.12:06:13
Notes copied from the task board:

Maybe have a script (maybe part of a new tox -e fix-style) with options to check, show diff without changes, and modify (refuse to modify anything with there are uncommitted changes).

The buildbot workers still install mercurial and set up "hg uncrustify". Once we have an alternative (and they don’t have to pull from anymore), we should update them as well.
msg9369 (view) Author: jendrik Date: 2020-07-01.11:07:02
We want to find a replacement for "hg uncrustify -m".
Date User Action Args
2020-07-03 09:53:42jendriksetstatus: reviewing -> resolved
messages: + msg9428
2020-07-02 19:41:52maltesetmessages: + msg9423
2020-07-02 17:58:51floriansetmessages: + msg9415
2020-07-02 17:52:45jendriksetmessages: + msg9414
2020-07-02 17:04:34floriansetmessages: + msg9408
2020-07-02 16:31:15maltesetmessages: + msg9404
2020-07-02 15:13:50jendriksetmessages: + msg9402
2020-07-02 00:59:54jendriksetmessages: + msg9397
2020-07-01 19:11:27jendriksetmessages: + msg9386
2020-07-01 16:07:26maltesetmessages: + msg9381
2020-07-01 12:39:26jendriksetstatus: in-progress -> reviewing
messages: + msg9378
2020-07-01 12:06:13floriansetnosy: + florian
messages: + msg9372
2020-07-01 11:07:03jendrikcreate