Nacho/fix flake8 remaining tests

[nachovizzo]

Fix flake8 linter

This PR is a follow-up #3893, and assumes #3941 was merged (to reduce the clutter in the diff)

Real diff (after black)

Assuming #3941 was NOT merged, the diff between this PR and that branch can be seen here: /~https://github.com/nachovizzo/navigation2/pull/1/files

Proposal about atomic PRs

If you guys consider it necessary, after the CI passes and we know that this bloody linter is happy I can split this PR into multiple ones addressing 1-flake8-linter-plugin-check-per-PR. It's up to you

Description of contribution in a few bullet points

• I tried to be as atomic as I could in commits, so the reviewer could go commit-by-commit instead of looking at the entire diff
• I could not identify a REAL logic change in the code base. So far this was torture :)

About the new additional linters

The new linters were introduced (as I guessed in #3893) in this PR: osrf/docker_images#641. The question is if they make sense or not. I couldn't find a good justification for why we now have python3-flake8-quotes which to me, is the most useless linter I've ever used in my life. If it wasn't for this, just having black on the CI will guarantee good code format forever.

It took me a few hours of scripting and manual editing to make that thing happy, and I still see no benefit.

If the decision is to keep it for nav2 is out of the scope of this PR. I just wanted to provide the solution for the CI so developers can trust the build again, but I'd strongly advise against flake8-quotes

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

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (ea1902e) 90.33% compared to head (e17b196) 90.37%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3942      +/-   ##
==========================================
+ Coverage   90.33%   90.37%   +0.03%     
==========================================
  Files         415      415              
  Lines       18443    18443              
==========================================
+ Hits        16660    16667       +7     
+ Misses       1783     1776       -7     

see 4 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[nachovizzo]

By the way, just as an additional piece of information: the first test I could catch failing is in #3877 which dates from 16 Oct, a few days after osrf/docker_images#641 got merged (the 6th of October). Which makes sense to me

[SteveMacenski]

I very looked in detail in the first couple of files and briefly scanned through a few randomly sampled other ones here. Did you only lint for all of them or anywhere did you reorder / change anything other than formatting? If just formatting, I can merge without a deeper look

[nachovizzo]

This block you can ignore for sure:

This as well, is just the linter saying "Add a . at the end of the sentence in the docstrings" and stuff alike,

If you skip those, then just clicking on the first few commits will show the "changes" which are basically formatting, removing unused imports, etc.

@SteveMacenski please mind that this PR also includes #3941 (first commit). So if we don't want to add black here, then please DO NOT MERGE THIS PR

[SteveMacenski]

I would like to have a PR that is just doing what is needed by OSRF's profile of flake8 to make CI turn over. Any conversation / changes about Black, if those changes are not overlapping with flake8, should be isolated to another PR / discussion.

[nachovizzo]

I would like to have a PR that is just doing what is needed by OSRF's profile of flake8 to make CI turn over. Any conversation / changes about Black, if those changes are not overlapping with flake8, should be isolated to another PR / discussion.

I agree :) all good. But it would take me a bit more time, I need to find again a spot where I can spend some time fixing by hand the thousand style errors like the following:

./nav2_common/nav2_common/launch/has_node_params.py:50:3: E111 indentation is not a multiple of 4
  def describe(self) -> Text:
  ^

, since I could not find any formatter that just does what ament_flake8 is asking for, maybe some folk out there have a better solution (?)

NOTE As a matter of fact, this PR in its current state, is not violating ANY ROS python style, that's why all the flake8 tests are passing. Even if we stop using black after this PR gets merged, then style errors would be much easier to catch and fix, and we can leave black behind, like the tool that helped us to fix the CI ;)

I leave it to you if you want to merge it or not.

[SteveMacenski]

thousand style errors

CI told me it was 133, but I get the point ;-)

Even if we stop using black after this PR gets merged, then style errors would be much easier to catch and fix, and we can leave black behind, like the tool that helped us to fix the CI ;)

It passes so yes. So you're saying that black has the same formatting as flake8's ROS 2 configuration? If so, we can use black in our CI potentially to auto-reformat (or at least run the option so that it can be handled). I thought you implied though that this was not the case when you closed the black PR.

[nachovizzo]

hehe, I like to be a bit dramatic 🤪

So you're saying that black has the same formatting as flake8's ROS 2 configuration?

No, I'm not saying this. I'm saying that black + the changes in this PR have a ROS 2 happy configuration. If you checkout my branch and do ament_black --reformat . you will see that black will attempt to change some stuff.

What I wanted to say, is that there might be a few lines where black is changing the style, even when there was no violation to flake8 linters. Nevertheless, there is still no rule-breaking after all. I can't tell which line particularly falls into this category, which might NOT STRICTLY be REQUIRED by ROS2 styling, but also NOT VIOLAINTG ROS 2 styling.

I closed #3941 because I believed that there was no interest in moving forward and let this open just for discussion (assuming black has no chance at all to interact here, then both PRs are useless)

If you ask me, I would merge first #3941, just to keep a git story clean (in case one needs to do criminology in the future), and then this one. After the CI is happy, the next time there is a linter problem, it would be only in the files developers are changing in Python, so fixing this would be straightforward.

Again, I leave this to you ;)

PS: In case I was not clear (which seems to be the case). As long as ROS 2 still commits to We pick single quotes over double quotes as long as no escaping is necessary. (source) , There is NO ROOM for black. It is a shame, to leave the most popular Python formatter out, just because, (we) prefer single quotes over double.

If we avoid this rule, then black can help to automatically format all the python3 scripts, and comply with flake8 guides (yes some rules can still be violated, but could also be fixed with isort and docformatter)

[SteveMacenski]

@nachovizzo let me follow up with some folks about this and get back to you. It didn't always used to be this way, so the fact that it can auto reformat might push some support black's way if proposed

[SteveMacenski]

Thanks for your time and energy to fixing this! This is a good first contribution to Nav2 and really saves me some time to focus on other elements. Your time is very appreciated :-)

[nachovizzo]

@SteveMacenski thanks to you for the time and patience on this PR! I'm more than happy to help. And I'd be eager to support any effort for pushing for black. If it hasn't happened yet, all Python tests in rolling will start failing overnight, for example ament_black:

AssertionError: Found 91 code style errors / warnings:
  ./setup.py:3:16: Q000 Double quotes found but single quotes preferred
  ./setup.py:7:13: Q000 Double quotes found but single quotes preferred
  ./setup.py:8:37: Q000 Double quotes found but single quotes preferred
  ./setup.py:10:10: Q000 Double quotes found but single quotes preferred
  ./setup.py:10:36: Q000 Double quotes found but single quotes preferred
  ./setup.py:11:10: Q000 Double quotes found but single quotes preferred
  ./setup.py:11:56: Q000 Double quotes found but single quotes preferred
  ./setup.py:14:9: Q000 Double quotes found but single quotes preferred
  ./setup.py:15:9: Q000 Double quotes found but single quotes preferred
  ./setup.py:16:9: Q000 Double quotes found but single quotes preferred
  ./setup.py:17:9: Q000 Double quotes found but single quotes preferred
  ./setup.py:20:12: Q000 Double quotes found but single quotes preferred
  ./setup.py:21:18: Q000 Double quotes found but single quotes preferred
  ./setup.py:22:16: Q000 Double quotes found but single quotes preferred
  ./setup.py:23:22: Q000 Double quotes found but single quotes preferred
  ./setup.py:24:9: Q000 Double quotes found but single quotes preferred
  ./setup.py:25:18: Q000 Double quotes found but single quotes preferred
  ./setup.py:26:15: Q000 Double quotes found but single quotes preferred
  ./setup.py:28:9: Q000 Double quotes found but single quotes preferred
  ./setup.py:29:9: Q000 Double quotes found but single quotes preferred
  ./setup.py:30:9: Q000 Double quotes found but single quotes preferred
  ./setup.py:31:9: Q000 Double quotes found but single quotes preferred
  ./setup.py:33:17: Q000 Double quotes found but single quotes preferred
  ./setup.py:37:13: Q000 Double quotes found but single quotes preferred
  ./setup.py:38:20: Q000 Double quotes found but single quotes preferred

....

[SteveMacenski]

If it hasn't happened yet, all Python tests in rolling will start failing overnight,

Wait, what? This wasn't supposed to introduce black or any of its formatting. Why would CI be failing?

[nachovizzo]

If it hasn't happened yet, all Python tests in rolling will start failing overnight,

Wait, what? This wasn't supposed to introduce black or any of its formatting. Why would CI be failing?

Sorry Steve 🤦 It's me being unclear again.

1. What I wanted to say is that a lot of packages (like nav2) will start to see errors (if they use linters) on their CIs because of this double quotes, etc. I took the ament_black package for example (forget about black, it's just a coincide now :P ) that was rolled out few weeks ago, and now when I added the package in rosdistro for the rolling distribution the "tests" (linters) started to fail due the double-quote/single-quote war. As matter of fact, I checked the CI before from nav2 and it was working, but once we pulled these install flake8 and pytest plugins from apt instead of pip osrf/docker_images#641 changes into the container being used to test with the CI, then out of nowhere you started to see those hundred errors. I was just trying to be a bit more pushy on this double-quotes discussion ;)
2. If you are referring to the current situation with the nav2 I think the errors are not related to the linters, but looks like a failure in codecov and the nightly run, but not due flake8 complaining, the style tests still passed for the main branch:
   

[SteveMacenski]

I still don't understand why anything would be failing with ament_black being released to rosdistro -- its still not part of the default linter set, correct? That would involve OSRF and/or the repo associated with ros2 testing

[nachovizzo]

It's not part of the default linter set.

But since in rolling the flake8 plugins 'appeared' recently (as I explained here: #3893) then packages that where "following| the style guidelines would start to fail IF there are double quotes. Just like happened in nav2