add code formatting target to makefile #2760
Conversation
…and format-python target (black formatter)
|
I am not sure that I have an opinion on this. |
|
I'd also be interested in opinions from the community.
Not impossible but it certainly adds a step. It's a price to pay and I currently don't know any way around it. For the meantime, there's
|
|
In theory, for code comparisions, it should be possible to pipe the old code through clang-format before creating the diff, but I'mt not sure that can be automated with git / github. |
|
I am convinced that source code formatting is important to attract new devs and to make a good first impression on whoever is making decisions to adopt LinuxCNC. So, I would go for it. Concerning diffs, I think we need do talk with GitHub about that. |
|
Personally I do not believe the negative consequences of a large
reformatting is compencated by the increased readibiliy, so I would
rather update the code style and introduce any formatting errors in
small steps once any code needing reformating was touched for other
reasons.
If we do not take that route, I prefer to do any reformattings as
separate commits.
…--
Happy hacking
Petter Reinholdtsen
|
What's the advantage here? I can't see any but think, that it makes a nasty process even more complicated.
Isn't that exactly what you want to prevent? It's very easy to confirm a commit that has only reformatting (astdiff must be empty) and you'd want a clean diff without format changes for any commit with semantic changes. Personally I don't care about diffing very old code with properly formatted newer code. It's a solvable problem and a rare edge case.
I have yet to find a part of the code (.c, .cpp) that is properly formatted. I'm using vscodium and pretty much every file contains wrong/misleading indentation. |
|
[heeplr]
What's the advantage here?
One advantage is that 'git blame' and 'git log' only show relevant lines
changed for each commit, making it easier to figure out why code lines
are the way they are.
Isn't that exactly what you want to prevent?
Not really. If the code line change anyway, it does not make much
difference to 'git blame' and 'git log' if its indentation alsy change.
I have yet to find a part of the code (.c, .cpp) that is properly
formatted. I'm using vscodium and pretty much every file contains
wrong/misleading indention. For python it's easy since there's pep8
as official formatting standard which lots of our files violate.
Which code style do you use as your baseline for 'properly formatted'?
The descriptions in docs/src/code/style-guide.adoc and src/CodingStyle*
are slightly different, so it is a bit unclear to me what the target
formatting should be.
…--
Happy hacking
Petter Reinholdtsen
|
Yes, that's basically the problem @andypugh mentioned. It affects all reformatting that isn't white space only. But I can't see an advantage of many small commits instead of a single big commit. It's just shifting the same problem into the future. Plus it adds logistics like "please separate your reformatting commit and your semantic change commit before submitting a PR" (how to communicate that?). And code review needs to pull out astdiff over and over again until everything is according to standard eventually.
But that line will drown in other lines where just the formatting changed if commits are not cleanly separated. Or are you suggesting to format just parts of a file? omg that would introduce even more mess and can't be automated. Contributors would need to do the work of clang-format manually. I really couldn't see that in practice.
Short answer: "properly formatted" is no matter of choosing a style or ruleset but rather anything that's 1. enforcable in CIand 2. consistent across the codebase.Longer answer: Currently, the .clang-format config of this PR is based on @rmu75's work with minor changes and lots of disabled options with the goal to
I tried to stick to the clang-format defaults as close as possible with respect to the above aspects. The final ruleset will hopefully be a result of this discussion and there's no trouble changing it in the future and reformatting everything again. But I really don't have an opinion on the final style as long as it's consistent and enforcable.
Yeah, those would be obsolete (they obviously are already) and might be by replaced by a short note like "run |
|
[heeplr]
Or are you suggesting to format just parts of a file?
Only the changed lines in the file, yes. Might not be the best idea,
but it is an alternative. :)
Yeah, those would be obsolete (they obviously are already) and might
be by replaced by a short note like "run `make format` before
comitting".
OK. Good to know your proposal is to replace the current style guides
with something different. It might be easier than figuring out which
part of the two slightly different style guides to keep. I have no idea
how attached the project is to those style guides.
…--
Happy hacking
Petter Reinholdtsen
|
Working with the code until now gave me a pretty good idea: No one gives a darn. ;) |
|
IMHO it all depends on what we want to achieve with our code. The doxygen changes (#2827) and the code formatting both help to make LinuxCNC look more professional and likely help new devs to become productive more quickly. I personally also see a certain beauty in an indentation that perfectly and reliably matches the semantics. Anything else to me is like a typo in the documentation, or worse. You just want to fix this. Git blame and diff can work across multiple revisions, so I am not completely worried. Also, I am more of a bisect person than a diff person. And if truly interested to perform a diff against particular branch, one can still apply the formatting to that branch/revision no matter how old that branch/revision is. I very much get that anyone deep in the development of LinuxCNC will not necessarily care about the beauty of the code. The community contributing the reformatting and an automation of that process via git hooks should be happily embraced, though. |
|
I haven't found much code where the intentation is wrong, as long as you experiment a bit to see what the original coder wanted to do. The main culprit here are the files that use tabs and spaces and these look wrong until you set tabs to 8 spaces. I feel that losing the "blame" view in Github would be a significant sacrifice. It's super-useful for things other that bisect can't do, like checking if changes properly propagated, and if so, which version. |
This is not just about indentation. Formatting also includes braces placement, (non)spacing etc.
Yeah, I don't really feel like experimenting. And I don't think anyone should be bothered with such a time waster. It's certainly not a good strategy in terms of working on the project.
It's not the "main culprit". It's one flaw among many.
I never said it's a huge problem but it's probably the lowest hanging fruit on a way to a clean & modern codebase.
Understandable. They're not very inviting.
I guess that's what If you care about the github webinterface, please consider this solution. Please Remember: And I really like to stress (again) how backwards the current state is. IMHO this discussion should focus on "how it's done" not "if it's done". This is just my suggestion for a first draft to tackle the problem. Considering other, much larger issues in the linuxcnc codebase - which require a lot of work and a lot of important decisions - a thing like "automating code formatting (rule enforcment)" should be easy. |
|
Hello @heeplr, It took a while, but our review of PRs in today's spontaneous video meet-up had us agree that any automatism to get our source tree into a shiny nicely formatted consistent state would be good, indeed. The only concern is that any such dominating set of changes will ruin what |
|
Should be easy to just test what is suggested in the solution heeplr linked It is possible to instruct github blame and also |
|
I missed that. Thank you, @rmu75 . No immediate idea about how to proceed from here, though. |
|
Is the idea here to add continuous automatic reformatting, or an option to reformat as needed or ?? The first one sounds pretty horrible and the second seems troublesome too. What are we trying to fix? I feel the medicine might be worse then the sickness. |
|
At least on the C/C++ side, linuxcnc source code is pretty consistent. IMO it would make sense to reformat the code that doesn't adhere to style conventions in one commit (which will be small) and automatically apply clang-format upon commit / pull request. That could also make editing code in modern IDEs like VS code/codium more convenient because they can automatically use clang-format settings and nobody has to think about it any more. I don't know about python code. Personally I use black to have consistent python style, but I admit the result can look strange. |
I am not sure that I agree with this. Many files are formatted with mixed spaces and tabs, whereas others are all tabs or all spaces. |
at least it looks consistent if the expected tab length is used (4 i think). |
|
Could you explain this more please: |
I did not read about any auto-applied formatting in the PR. But repeated white-space commits are to be avoided, hence we need some way to a) format everything how it should be in a single huge commit (to then be masked from git blame) and b) only accept properly formatted commits afterwards. No idea if any git hook would be beneficial or if just the CI should fail. I am confident we could all benefit from a such introduced consistent coding style. It’s not something that will directly fix existing issues, but I expect it would make the codebase easier to navigate and maintain. Even if it doesn’t immediately attract new contributors, it could make reviews and external engagement smoother and strengthen the overall impression of LinuxCNC as a well-structured and professionally maintained project. Could you explain what exactly your concerns are? |
I would do it with a git pre-commit hook like FreeCAD, explained here https://freecad.github.io/DevelopersHandbook/codeformatting/ That would guarantee consistent formatting. |
|
Thanks for replying. First reasonably consistent formatting is a worthy goal. Getting rid of all tabs would be great IMO. For instance we have a version of classicladder in the linuxcnc code. It would be great to have it's code consistently formatted but it doesn't have to have the exact same style as the mesa hostmot2 code. I do a lot of python coding, While I could surely use some more consistent formatting, I certainly don't want to perfectly follow pep8. I absolutely don't want automatic format changing for code I commit and I don't want code automatically blocked because it doesn't perfectly follow some formatting rules. As the developers that have commit authority, we are the ones who should decide it the formatting is so bad it needs to be fixed. But the truth is we are so short on developers and pull requester that we can ill afford to chases away code If we want to fix formatting in the code, lets fix it in one big push and they leave it alone. |
|
I certainly do not want to impose anything upon anybody. But. Some of the C++ code is really hard to follow in github or with wrong tab size configured (e.g. interp_G7x.cc or those nested switch/case statements in emctask*.cc) so IMHO "fixing" that will really lower the barrier to understanding and participation. I don't have nor want commit access, nevertheless after kludging zmq into emctask and tinkering with the TP I would argue that there are indeed places where the situation is bad enough it needs to be fixed ;-) Even fixing only the github-display should be reason enough. Just as example this is unreadable in github: class straight_segment:public segment {
public:
straight_segment(double sx, double sz,double ex, double ez):
segment(sz,sx,ez,ex) {}
straight_segment(std::complex<double> s,std::complex<double> e):
segment(s,e) {}
void intersection_z(double x, intersections_t &is) override;
bool climb(std::complex<double>&,motion_base*) override;
bool dive(std::complex<double>&,double, motion_base*,bool) override;
void climb_only(std::complex<double>&,motion_base*) override;
void draw(motion_base *out) override { out->straight_move(end); }
void offset(double distance) override {
std::complex<double> d=I*distance*(start-end)/abs(start-end);
start+=d;
end+=d;
}
void intersect(segment *p) override;
void intersect_end(round_segment *p) override;
void intersect_end(straight_segment *p) override;
std::unique_ptr<segment> dup() override {
return std::make_unique<straight_segment>(*this);
}
double radius() override { return abs(start-end); }
};This code was formatted with an indentation of 4 characters and a tab-size of 8 characters whereas github seems to advance a tab only 4 spaces. I'm less sure of consequences of using something like "black" on python code, e.g. using it on axis generates a large number of changes from 'single quote style strings' to "double quote style strings". AFAIU a pre-commit hook would happen on the developer machine, so if one were to use that it would assure consistent formatting among all branches from that dev. I don't know how to deal with cherry-picking or old branches. In theory, it should be possible to apply the formatting to the old branch / cherry-picked-commit before merging, but I don't know if that works in practice or if there is some automated support of that in git or github. Automatic formatting would take care of "not perfectly following a formatting style". OTOH requirement to install some pre-commit tooling is an additional hurdle. Failing CI because of formatting / style problems is a bad idea IMO. In personal projects, I configure emacs / vs code to automatically invoke clang-format resp. black when saving a file. Styling should probably not be applied to imported code like classicladder (but last I looked upgrading classicladder would amount to completely re-do the integration again because of extensive changes on both sides since branching off the common ancestor). I will take a closer look how FreeCAD implements per-subsystem opt-in/opt-out to automatic formatting. |
standardization > personal taste
Why not? The upsides outnumber the downsides and nothing is blocking you from re-formatting to your own taste automagically after each pull/commit and using your own style. Just make sure to run the provided formatter before committing (could be automated).
I'd say that another constantly failing test which no one has the time to fix, is a very bad idea.
Exactly. But you decide that once when you agree on rules. Not for each and every commit or PR.
You could offer a nice way to format correctly so no one has to worry about that. Ever again. As mentioned above I'd argue the opposite:
Do you have an example for frustration about wrong style? Normally this is done automatically. (If you use some common style, changes are subtle most of the time.)
What about the future then? Would you volunteer to fix wrong style regularly? It just makes so much sense to automate this. |
If github rejects a push with style-breaking commits, one could certainly set up a pre-commit hook. The important thing here imho are clear instructions. If there's a nice message like
Imho it wouldn't hurt to double check if there's a mechanism to ensure, that failing CI is the exception caused by weird edge cases or bugs. If a git hook ensures coherent style, that CI test will probably never fail unless the hook is buggy or not triggered. |
Have a look at src/hal/drivers/hal_motenc That is one of many files formatted for 8-space tabs where the indent levels are space space space space and so on. They look sort-of OK at 4 spaces per tab, but better at 8. I am unsure if the code-tidying tools can handle this case. |
|
Mixing tabs and spaces is really very annoying, doing indentation with mixed spaces/tabs on the same line is evil.
clang-format is a compiler that produces source code as output. What I want to say is that clang-format is not some dumb tool that superficially does some guesswork like IDEs used to do but it really understands the code. You can remove/add non-essential whitespace however you like and it will always reformat into the same result. (At least it can be configured to do that afaiu, maybe there is also a "don't care mode" that can leave some stuff as is). |
This, like any other external code, to me is actually a good example for which I would indeed see problems with the idea to perform any code reformatting, since the challenge here would be to adopt updates from the external-to-us upstream source tree. |
|
I would agree with c-morley. I have had the experience a couple of times where a fix was kicked back multiple times for minor code style issues. It was not automated, and the admin spent more time kicking it back then it would have taken him to fix the petty stuff himself, and I wound up spending more time fixing them than I did fixing the bug in the first place. This to me is a far greater turnoff than minor differences in code style. |
7 participants
This PR adds 3 new targets to the Makefile:
format-python- that uses black to format all python code withinsrcformat-cpp- that uses clang-format to format C/C++ files withinsrcformat- to combine the above targetsthe
.clang-formatconfig is based on @rmu75's work with minor changes and lots of disabled options with the goal toI'd like to suggest the following steps: