Repository navigation
RFC: Coding style check/enforcement #4223
Description
Activity
I left include file sorting out because almost no files have them sorted alphabetically now, though I think it would be good nonetheless
How includes are ordered now is by "logical groups", where system headers come first, then empty line, then uPy includes, kinda in some logical order too (from most basic ones like py/obj.h to more specialized). That's not too consistent of course, but artificial order like alphabetical won't do any good.
More involved solutions woule be things like pre-commit hooks or adding a check for PRs. Ideas/comments?
I personally think that checking title/commit message PR is more important, even following fail-fast idea. Majority of PRs received don't follow the guidelines. But I also personally think that first existing regressed infrastructure should be fixed, e.g., #4026 .
The differences are mainly with respect to line length/
Lines are known to be long, 100 as in your config are not enough.
breaking arguments
Should enumerate existing examples and decide which to follow.
and indentation of PP directives.
Yes, there're number of cases which by the latest conventions should be indented.
How includes are ordered now is by "logical groups"
The groups are ok, clang-format can support (and enforce) those. It's just the order within the groups which would be sorted; alphabetically by default (but can be arbitrary regex-based). I don't agree it does no good though: it's consistent, doesn't require thinking and trying to derive whatever system seems to be commonly used then trying to implement it only to end up with something random anyway (as it is now)
I personally think that checking title/commit message PR is more important, even following fail-fast idea.
Well, yes, but that happens now already, manually because it requires en understanding of semantics/reasoning/meaning no tool can do. Are you suggesting an automatic PR check? I guess a regex can get out the most basic mistakes and would definitely help with the "fixed a bug, see code"-style messages we get sometimes.
Lines are known to be long, 100 as in your config are not enough.
Should enumerate existing examples and decide which to follow.Yeah I just entered something rather arbitrary like 100 because that seems to be intended 'most of the time' but that is just a guess..
See argcheck.c for example: there are function declarations of about 150 columns while at the same time all nlr_raise statemements are broken up to not go past 80 (?) columns even when they wouldn't go past 150. But a search for nlr_raise shows a mix, even within the same file, of single-line nlr_raise statements extending up to 140 columns.Thanks @stinos for providing the config options for clang-format. Looking through my old notes it turns out I did actually try clang-format myself, and came to this definitive conclusion: "not clang-format, it requires installing all of clang, and can't do indented preprocessor directives". The first limitation (requiring a full install of clang) is not too bad. But as far as I could work out, it cannot handle indented preprocessor directives, so all those would need to be shifted back to start at column 1. That's pretty much a deal breaker in my mind.
Running clang-format on py/runtime.c with the above options shows most of the issues with it:
- removes space around inline curly braces, eg
.base = {&mp_type_module}, - adds a space before
*used as a pointer, eg.globals = (mp_obj_dict_t *)&... - dedents preprocessor directives
- incorrectly reformats
//comment blocks with long lines - dedents some
//comment blocks (seems to be those before a preprocessor directive)
Some of the stuff it does with comments is strange, eg removing a space within a comment (to fit it within 100 chars column width):
// We need to create the following array of objects: - // args[0 .. n_args] unpacked(pos_seq) args[n_args .. n_args + 2 * n_kw] unpacked(kw_dict) + // args[0 .. n_args] unpacked(pos_seq) args[n_args .. n_args + 2 * n_kw] unpacked(kw_dict) // TODO: optimize one day to avoid constructing new arg array? Will be hard.
Unfortunately clang-format doesn't look very good for this project.
For the record, for astyle, here are the options I found which minimise the diff when run on the py/ directory:
$ astyle --options=astyle.fmt *.c astyle.fmt contains: --style=google --indent=spaces=4 --attach-closing-while --add-brackets --indent-switches --indent-after-parens --indent-labels --indent-preproc-define --indent-preproc-cond #--pad-oper --pad-header --unpad-paren #--align-pointer=name --add-braces --convert-tabsBut note that, even with these options, astyle just can't handle py/vm.c.
- removes space around inline curly braces, eg
Fair enough. Note you can get clang-format without the whole of clang when using the pre-built binaries, though that's a manual thing to do. Anyway I know there's still some bugs in it and probably the missing option for the PP directives might also be because clang-format is still relatively new..
For the curly brackets: I actually wondered why they have a space, given that none of the other brackets do?For the curly brackets: I actually wondered why they have a space, given that none of the other brackets do?
It's not a big thing (and not 100% consistent), but curly braces used for function definitions (especially one-line inline functions) would have a space on the inside of the braces. So that style extends to struct initialisers.
@stinos I've somewhat shifted my thinking on this since you brought it up. For long term maintenance of the project I think it would be good to enforce a consistent coding style automatically, even if it means a bit of noise (changing code) to get there.
We could write our own style formatter (in Python...) but probably that's a waste of time/effort. If anything just go for astyle and accept that it'll make some changes.
Reacted by stinosI tried astyle but either my version (3.1) is broken or I don't understand the documentation on
--indent-after-parens: instead of leaving something likenlr_raise(mp_obj_new_exception_msg_varg(&mp_type_TypeError, "blabla")alone, it adds extra indentation to that last line so there are 8 spaces before "blabla". But with
--indent-continuationset to 0 (1 is default), it removes all indentation. It should work in multiples of 4 spaces, right?I don't understand the documentation on
--indent-after-parensIt looks like it adds an indent (4 spaces) for an "=" and for each "(". Eg:
foo(1, 2, 3, 4); // one level of indent because "(" on line above x = foo(1, 2, 3, 4); // two levels because of "=" and "(" on line above x = foo(1, bar(2, 3, 4); // three levels because of "=" and 2x "(" on line aboveAt least it's logical!
If we do go for astyle,
py/vm.cwill need to be excluded because it just can't handle the big switch.At least it's logical!
Aha, now I see. I guess if we can live with the major initial change this introduces, afterwards it will all be good. In fact I did see astyle fixing some cases which are effectively 'wrong' already. Adding some exclusions shouldn't be a problem.
Slightly related: we should do the same for all Python code. But that's much easier to format. (And including linting as well to catch errors as in a recent PR is also not a bad idea)
@stinos I've somewhat shifted my thinking on this since you brought it up. For long term maintenance of the project I think it would be good to enforce a consistent coding style automatically, even if it means a bit of noise (changing code) to get there.
PR #5310 reminded me to look into this. My personal opinion is that the benefits of consistency and the huge amount of convenience from having this done automatically far outweigh the small cost in getting there (diff/noise and maybe changing some of the rules).
I took a look at the results from running both astyle and clang-format. As already discussed, preprocessor indentation is a non-starter for clang-format, but astyle doesn't get it quite right either. It particularly struggles when it's being used to conditionally enable entire branches of an if/elif/else block or immediately before a case label. It's not that severe though, and like @stinos says it overall fixes more things than it breaks.
However, is it worth considering changing the PP indentation rule to "always-start-of-line" in order to make this issue go away? (I much prefer it that way anyway, regardless of what the tools support, and I think it's generally a more commonly used style [citation needed other than just the Google Style Guide :p ])
It seems like astyle overall does do a marginally better job. I wish I could figure out a way to make it handle those
nlr_raiselines better though. There are plenty of other existing places where the alignment is to the opening bracket, so perhaps not setting--indent-after-parensis better and making these lines consistent (but quite long) would be better. In conjunction with--max-continuation-indent=40it prevents it getting too out of hand.Here's the config I was testing with (.astylerc):
--style=google --suffix=none --indent=spaces=4 --attach-closing-while --add-brackets --indent-switches --max-continuation-indent=40 --indent-labels --indent-preproc-define --indent-preproc-cond --pad-header --unpad-paren --add-braces --convert-tabs --keep-one-line-statements --keep-one-line-blocks --max-code-length=200In the short term it would be good just to have a
tools/code-format.shbut would be nice to see it eventually become a travis step or something.#!/bin/bash if [ ! -e .astylerc ]; then echo "Run this from the top-level directory, e.g. ./tools/code-format.sh" exit 1 fi EXCLUSIONS="py/vm.c" FILES=`ls py/*.{c,h} extmod/*.{c,h} ports/*/*.{c,h} | grep -v -F "${EXCLUSIONS}"` astyle --project ${FILES}
Reacted by stinosHowever, is it worth considering changing the PP indentation rule to "always-start-of-line" in order to make this issue go away?
For MicroPython I can actually see why start of line is not used, it can improve readability: 4 spaces for indentation combined with sometimes rather deep nesting and quite heavy use of conditional PP, so the actually guarded code gets quite 'far' away from the PP directive. Then if you use a rather plain text editor it's easy to miss things.
Still I'm like +0.5 on considering this if it's the major hindrance in adopting automatic formatting. Because as mentioned: it's going to be worth it. Not just consistency but also for reviewing etc; I've been doing more code reviews than usual myself lateley and found it gets extremely frustrating and tiring to have to repeat 'wrong indent/missing space/wrong capitalization/...' over and over again when you'd rather focus on the actual code. Repeating 'no further review until it's been ran through the formatter' is much less work then :)
I'd be happy to adopt the same code formatter in CircuitPython as well.
Does astyle add curly braces around single statement blocks? It is a huge pet-peeve of mine because it can lead to serious bugs and clang-format doesn't actually do it, only clang-tidy can.
Any concern that astyle isn't being developed anymore? The last commit to its repo was April of last year.
I have an oldish CircuitPython branch with clang-format and clang-tidy settings here.
Does astyle add curly braces around single statement blocks?
Yes. That is one of my favorite options. The output style of astyle is highly configurable. A little bit confusing at first glance.
Any concern that astyle isn't being developed anymore?
Although it's rare, some software tools can actually be "finished" and not need updating :) Eg TeX/LaTeX. The C language doesn't really change so a formatter has potential to also not change.
I'd like to add a +1 to this proposal, although I can't contribute any specifics. All I can say is that when programming in Go I got used to having the reformat happening automatically upon save in VIM and it's such a blessing in large projects. I absolutely hated some of the formatting details (it's 100% non-configurable!) but had to agree that the benefits far, far outweigh my grumblings.
Ok, so let's try to find a style and formatter that's acceptable. And would be great to agree with CircuitPython so we use the same.
astyle is fine with me. We can always move to clang-format/clang-tidy if we discover they do something we want.
I'd go for a style that produces minimal changes on the existing code.
Oh, and we should make sure we can automatically check that a PR matches the format with the CI.
For the Python code (mainly the tests, there are quite some inconsistencies there), any preferences: autopep8/black/yapf?
I found black to be a bit too opinionated, so I like to use yapf. I haven't really used autopep8.
blackis my preference because of its increasing popularity. I agree some of its choices look weird (like 1 param per line) but the motivation is clearer diffs which makes total sense. We've just started setting up checking forblackformatting in our libraries.clearer diffs which makes total sense
I always found that a bit of a workaround for lack of other tools. But if I'm not mistaken it will only do the 1 param per line if the line is long enough to begin with so at the turnover point you have a large diff again?
Last couple of months I've been viewing diffs mostly in SublimeMerge and the way it displays changes to function arguments etc (as compact as possible, I have the impression) is really clear btw.
clearer diffs which makes total sense
I always found that a bit of a workaround for lack of other tools. But if I'm not mistaken it will only do the 1 param per line if the line is long enough to begin with so at the turnover point you have a large diff again?
Correct, you have the large diff once as the signature grows.
@dpgeorge where are we on this? I'd love to get a formatter adopted in CircuitPython.
where are we on this? I'd love to get a formatter adopted in CircuitPython.
I was fixing a few things like ce39c95 which affect formatting, and also looking at clang-format in more detail. clang-format is quite opinionated and likes to make lots of changes. I'm really leaning towards using astyle, and enabling the
--indent-preproc-condto make the #if's line up with the code they are configuring.Let me make another astyle attempt on current master then we can make a decision.
Reacted by Scott Shawcroft
It's nice to be consistent and using a tool like clang-format is by far the easiest way to achieve that. Below are clang-format settings which match the envisioned style for MicroPython pretty well (I left include file sorting out because almost no files have them sorted alphabetically now, though I think it would be good nonetheless).
Based on some brief previous discussion there is interest to somehow incorporate that in MicroPython. This could be as simple as including a .clang-format file in the repository. People can then choose to just format new/modified pieces of code or whole files. More involved solutions woule be things like pre-commit hooks or adding a check for PRs. Ideas/comments?
The major problem with introducing this: the codebase right now isn't formatted 100% consistently. This is to be expected of course, but with an automated tool it's annoying. It's not bad but I randomly went over a couple of files and there's only a small percentage which are completely ok. The differences are mainly with respect to line length/breaking arguments and indentation of PP directives. There are again different ways to deal with this: leave as-is (drawback is running the tool on a whole file nearly always creates a diff), format everything at once, format chunks or files incrementally as they are modified, ...