| ROS Resources: Documentation | Support | Discussion Forum | Service Status | Q&A answers.ros.org |
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:
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
or
[flake8]
max-line-length = 88
extend-ignore = E203Those 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.
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.
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