One of the real frustrations with contributing and writing python code with the flake8 checker is that previously there hasn’t been a tool like clang_format to automatically strictly format python code for you. In conversations with a friend who works on a large python project I learned about just such a tool. https://github.com/psf/black
The largest change if we adopt it as a code formatter would be how it deals with quotes and how that differs from the current standard. It biases towards double quotes where currently we use flake8_quotes which biases towards single quotes. To use black we’d either have to configure flake8_quotes in ament_flake8 or remove it from the recommended list of flake8 extensions.
Lastly, it would useful to have a ament_black to check for black formatting just like we have an ament_clang_format. I’ll happily contribute this if we can accept black python formatting.
Thanks for opening this thread @tylerjw. I agree that black is a tremendously useful tool, and we’ve hit the quotes issue using it as well. I’d love to see an ament_black linter that runs black --check --diff for me, assuming we can get around the quotes issue.
It sounds more to me like “we need to upgrade black to be more configurable”. That’s a (current) shortcoming of the tool rather than a shortcoming of the linters. As they note early in the readme “this is a beta project”. We need to be very hesitant on average to throw away things that are working for us in favor of the newest shiny.
EDIT: Also --skip-string-normalization option at least skips it editing away from current linter standard.
That being said, looks like a really promising tool!
I think such configurability runs counter to black’s philosophy. It’s pretty uncompromising, which is part of the beauty. No one adores it, but no one has to fight with it; they just code. Note that I think we can also just pass --inline-quotes '"' to flake8-quotes to make black usable. I doubt @tylerjw is really suggesting that we get rid of linters that are working for us, merely trying to outline a path toward using/adding a new one.
I just pointed out the largest change I saw that black would require (single to double quotes and therefore changing the settings for flake8_quote). Like @kyrofa black is not configurable by design and that is a good thing. A non-configurable auto format tool offers all the benefits of clang-format without any of the useless debate around how to configure it.
As it stands now black is the only effective clang-format like tool for python. All of the others do too little and still allow much more variety in formatting and manual tweaking of files to pass the base flake8 style rules.
I’ll go ahead and be the dissenting voice in this debate. Is this really a problem? Does it really impede understanding of the code? Or to ask another way; does changing to a rigid style like this (as opposed to the semi-rigid style we’re already using) improve performance, features, or quality of the Python code?
My general impression is that this doesn’t provide much benefit, but I am interested to hear specific reasons why this would be an improvement.
My understanding of these sort of tools is they take away both the need and the possibility for there being any discussion about style.
This helps in getting devs to accept the style of a project (in a this-way-or-the-highway sort of way) and also lets devs write code in whatever style they prefer: in the end, everything will be rewritten in the style anyway.
So instead of getting humans to run flake8 (or a similar tool) and then manually fix whatever violations it finds, you run a tool which goes in and rewrites everything according to the one style and never have to worry about violating any styles.
Similar to how clang-format simply overwrites (and overrides) whatever you do locally for C++.
I see the time spent manually trying to fix flake8 errors as really annoying. That is what got me asking around for a clang_format like tool. One of the benefits of using black will be much more consistent style than people trying to manually achieve flake8, making much of the flake8 tests irrelevant because if we adopt black we can just test if it would change anything (like we do for clang_format).
Changing to an automated rigid style for python has the benefit of saving time and pain in developing in that style. Anything we can do to make development of ros2 packages nicer will be good for the whole community.
It might help to actually compare the two tools in detail. If there feature set is significantly different it is not only about style questions but if dropping certain checks would be desired.
I would expect that the various flake8 plugins currently used check for more than what black does (though I haven’t used the latter, this is only an assumption).
@dirk-thomas I’m not suggesting we drop flake8 for black, we just won’t need the style format parts of flake8 afterwards. There will still be parts like doxygen checking and unused includes that will flake8 will be needed for. My understanding from using both is that black will change everything it can automatically. It can’t write doxygen comments for you and does not remove code so unused includes will still have to be manually fixed. black will fix line length or include ordering issues for example.
I agree with this, but there is also cost to changing. Not only the one-time cost of running the code formatter (which is fairly low); there is also the cost of invalidating basically every open PR. These will all now need to be rebased and retested.
For what its worth, this is kind of the situation we are in for C++, where we have both cpplint and uncrustify. I find the quality of life there worse. While it is not impossible to please both, it can be challenging to find a lowest-common-denominator that work for both. I don’t relish repeating that experience on the Python side.
As an alternative, what if we keep our existing flake8 style, but find some tool that will apply it for us? I haven’t personally done any looking around for this, but I have to imagine such things exist.
I don’t think it does. If it does I’d be happy to adopt it. From what I’ve found black is the only one that offers strict auto formatting and does comply with basic flake8 with minimal flake8 configuration. (only flake8 plugin we use that it doesn’t play nicely with is the quotes one)
As to invalidating existing PRs, rebasing after an autolinter is something that can be scripted and is relatively painless. I can also provide this script.
As to the issues with uncrustify and cpplint, I don’t have a good solution for that other than to drop one of them (that is what I’ve done in my own projects).
This is not limited to pending PRs. It also needs to be considered how this affects different distributions:
Either it would only be applied to master - that makes porting patches significantly more effort.
Or all active branches are updated / converted at the same time (to continue making backports easy) - but that implies changing code style in an already released distro which isn’t appealing.
I created an initial version of ament_black here for my own use: GitHub - Timple/ament_black: Python auto formatting tool ament package. Note that the output format is ugly but it is functional (I stuff the diff output into a string in the xml and am not parsing it). It is based on ament_clang_format.
This would be my preference, although I agree that changing code style of already released code is not appealing. I do think it would be a positive thing to do for the community though.
For what it’s worth, black does suggest a few configuration options to de-conflict with flake8. See https://github.com/psf/black#line-length. Some of the configuration options for flake8 are:
Black can be adopted with the flake8 settings called out by @tdenewiler if we drop flake8_quotes as it is irrelevant if we adopt black as it handles quotes.
In summary if someone finds this, I will post a PR to ament_lint to add ament_black and ament_cmake_black and adjust ament_flake8 to allow you to use it with ament_black. If people want to use this in their own projects they will have an option to use my fork/PR to try it out. If it is accepted as the standard the owners of the main ROS2 project will have to decide how to deal with the existing code.
I will post back here when I submit that PR.
As another side, I find it a bit ironic that there is no CI running on the ament_lint repo. I’ll make a PR to a industrial_ci based travis to it. In the current state it will not pass it’s own tests if you run colcon test with it in your workspace.
The ament_lint does have PR testing using build.ros2.org. Unfortunately no jobs have been created for Foxy (which is the current master) yet. So PR testing only happens atm when you target the eloquent or dashing branch.
Don’t spend any time adding Travis CI for the repo backed by industrial_ci. That would very unlikely be merged.