Skip to content

Use ament_black as linter#3941

Closed
nachovizzo wants to merge 1 commit intoros-navigation:mainfrom
nachovizzo:nacho/add_black
Closed

Use ament_black as linter#3941
nachovizzo wants to merge 1 commit intoros-navigation:mainfrom
nachovizzo:nacho/add_black

Conversation

@nachovizzo
Copy link
Contributor

@nachovizzo nachovizzo commented Nov 5, 2023

Use ament_black to format python code

@SteveMacenski I'm only opening this pre-PR to continue #3893. The reason is simple, most python scripts have many style errors according to flake8 which can be automatically solved by using perhaps the most famous Python format, black.

I have another branch where I'm fixing the missing linting tests, BUT those contain changes in the logic. The diff between main and a branch that fixes style errors + flake8 errors would be impossible to read/ follow.

This one is straightforward, I just did the following (on iron),

git clone nav2...
cd nav2
ament_black --reformat .

The only prerequisite would be to use the linter is to install the https://github.com/botsandus/ament_black linter (Yes I'm the current maintainer as well)

sudo apt install ros-iron-ament-black # or sudo apt install ros-hubmle-ament-black

About rolling

rolling is still waiting to get its ament-black package: ros/rosdistro#38762 . In the meantime and as a temporary solution, I'm also maintaining a Python package that strictly follows the rosdistro package versioning and implementation, thus:

pip install ament-black

Future work that may be required in bullet points

I wanted to keep this PR as straightforward as possible, so in future also the commit associated with this PR is PURELY style-change, therefore;

  • If the maintainers agree to start using black (I'm sure many people would love to see this change coming to the stack) then I will open another PR, adding the linter as part of the CI tests

For Maintainers:

  • Check that any new parameters added are updated in navigation.ros.org
  • Check that any significant change is added to the migration guide
  • Check that any new features OR changes to existing behaviors are reflected in the tuning guide
  • Check that any new functions have Doxygen added
  • Check that any new features have test coverage
  • Check that any new plugins is added to the plugins page
  • If BT Node, Additionally: add to BT's XML index of nodes for groot, BT package's readme table, and BT library lists

@nachovizzo nachovizzo mentioned this pull request Nov 5, 2023
7 tasks
@SteveMacenski
Copy link
Member

We follow the best practices of ROS 2 styling so we wouldn't be open to using another format other than what ROS 2 recommends so that Nav2 can be a "model" codebase of when all best practices are followed, here's an example of what a good codebase would look like.

If black is configured to use the same profile as ROS 2's flake8 config, then that would be really useful to have something that reformatted things. I guess I need some clarity about the intent of ament_black with flake8: are you seeing it as in addition to; instead of; or something that we can use to autoformat (perhaps even as part of the CI jobs having a bot push reformat updates automatically)?

@nachovizzo
Copy link
Contributor Author

@SteveMacenski I guess then black is not an option.. as a matter of fact, black will always format with double quotes (I'm not the only one against flake8-quotes)

That said, we will never manage to marry black and flake8-quotes, as long as this linter is part of the ros tooling, then black has (sadly) no room to play here.

Please confirm if you would like me to close this PR and rebase #3942 without the black changes

@SteveMacenski
Copy link
Member

If black can be reconfigured to use ament_flake8's configuration profile, having something that does auto reformatting would be great! But yes, we are following OSRF's coding style precisely. If additional rules are placed on things due to black that don't conflict with ROS 2's formatting, that's fine. But we will always take ROS 2's styling first.

@nachovizzo
Copy link
Contributor Author

Then good life to black :)

Understood and follow the ideal 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants