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.
1 Like
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:
-S, --skip-string-normalization
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).
2 Likes
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.
1 Like
To re-iterate on this. I’ve changed my mind over if ament_lint is the right place for black. ament_lint operates per-package and that makes sense for then including the tests as part of ctest. To me, format linters (like black and clang-format) are a special type of linter that should be applied per-repo and not per -package. To accomplish this I’ve been recently using pre-commit which adds git commit hooks to format your repo per a repo level yaml config and then using a GitHub Action to check this format.
There are a couple things that are great about this approach. First, it removes much of the bike-shedding for PR comments as the style is fixed and not part of the discussion. Second, it makes it really easy for contributors to format their code correctly as pre-commit checks their code when they type git commit.
If anyone here is looking for examples on how to do this, you can find me on the moveit discord server or just look at the various moveit packages on github for examples.
2 Likes