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
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: https://github.com/tylerjw/ament_black. 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:
[flake8] max-line-length = 80 ... select = C,E,F,W,B,B950 ignore = E203, E501, W503
[flake8] max-line-length = 88 extend-ignore = E203
Those flake8 settings work so long as you don’t install flake8_quotes as encouraged here: https://index.ros.org/doc/ros2/Installation/Eloquent/Linux-Development-Setup/#install-development-tools-and-ros-tools
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.
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
Don’t spend any time adding Travis CI for the repo backed by
industrial_ci. That would very unlikely be merged.
Ok, thank you for the heads up. Does this mean that PRs shouldn’t target the master branch and should instead target eloquent?
No, pull requests should always target the latest / default branch. Someone will trigger manual CI builds for all platforms and post the links on the PR.
Then why doesn’t it pass locally with the master branch checked out?
You may have a different environment locally than the build farm builders have. A common example is some dependencies installed via pip when the build farm uses Debian packages, or the other way around. Note also that I believe the build farm has moved to using Focal now, so there may be a difference in operating systems as well, which means the Debian packages might be different versions than what you’re using.
If the dependency management of linter scripts can’t enforce the correct versions of the linters to test themselves can’t be enforced, that seems like a really bad situation for anyone who tries to develop new features and test their changes and a major failure of the dependency specification system.
My local environment for this is 18.04 following the instructions here for installing eloquent: https://index.ros.org/doc/ros2/Installation/Eloquent/Linux-Development-Setup/#install-development-tools-and-ros-tools. I installed dependencies using rosdep.
Once focal is released I will switch to development using it.
I would hope that linters themselves are stable enough to not have this problem, and have full backwards compatibility assuming your config file doesn’t change. shakes fist at python and pip
Thanks @tylerjw for starting this thread and
ament_black! Having a way to do automatic formatting in Python would be tremendous fixing style is always cumbersome and is a significant component of PRs iteration. A way to run that automatically on each PR that gets opened would be very convenient too.
As far as the style is concerned, it looks like a precise list of the kind of resulting changes to the ROS 2 code base would be useful to see what the extent of the difference with the ROS 2 style is. The quotes issue was mentioned but there are likely multiple others that would need to be made visible before a decision can be made.
Regarding the “previous distros impact” question, ROS 2 linters evolve from one distro to the next and it’s not uncommon that code from a distro doesn’t pass the linter tests of another distro (waving at uncrustify). Also I think (if at all possible) going for an automated reformating that agrees with the set of flake8 plugins currently used by ROS 2 would allow backports making flake8 happy and more consistent style moving forward.
An example for the quotes issue, black offers a way to avoid reformatting quotes:
from black help message:
Don’t normalize string quotes or prefixes.
This is not exactly the same as saying “use single quotes” as black will just not reformat string quotes at all, whichever type of quote is used, but this could be a compromise to preserve backward compatibility.
It could also be argued that a clean break can be made and switching to double quotes would allow auto formatting and improve consistency with the other languages of the ROS 2 core (C and C++ both using double quotes).
Some other parts of black are also being made more flexible to ease adoption, and could be leveraged to match more closely the ROS 2 style, for example the magic trailing comma.
Black is a great tool definitely filling a need and I definitely will use it on some projects. It is however a young and quilckly evolving tool. A full report on the ROS 2 codebase to see if and how it can be adopted as a ROS 2 linter would be valuable to improve chances of adoption (in addition to the flake8 plugins currently in use).
Just my 50 cents to this: Due to the fact that
pep8 code format conventions leave some room for interpretation
black formats some things like e.g. functions exceeding the max. line length in a way which is uncommon to many Python developers and which results in
pylint reporting warnings. There are other tools like e.g. yapf as well. Probably it’s worth trying some tools on a reasonably big and varying code base, potentially with different configs (especially
yapf allows a lot of customization) and to choose afterwards.