Skip to content

install flake8 and pytest plugins from apt instead of pip#641

Merged
mikaelarguedas merged 1 commit intomasterfrom
flake8_from_apt
Oct 6, 2022
Merged

install flake8 and pytest plugins from apt instead of pip#641
mikaelarguedas merged 1 commit intomasterfrom
flake8_from_apt

Conversation

@mikaelarguedas
Copy link
Contributor

Closes #640

Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>
Copy link
Member

@ruffsl ruffsl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for looking into this!

flake8-quotes \
pytest-repeat \
pytest-rerunfailures
argcomplete
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there still a reason that argcomplete is soloed out?

Looks like its also available upstream:
https://packages.ubuntu.com/jammy/python/python3-argcomplete

I couldn't find what the build farm does for this, given argcomplete probably isn't used by the buildfarm.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For a long time the version of argcomplete was too low for several ros2cli and/or colcon features.
Maybe things have evolved since. Looks like in official documentation it switched to apt for that as well: https://github.com/ros2/ros2_documentation/pull/1275/files.

On the other hand if the list of pip packages to install is empty the template will stop installing pip or setuptools altogether that may have side effects so I'd rather not do this here and solely fix the flake8 issue in this PR

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good.

@mikaelarguedas mikaelarguedas merged commit ac5d1e8 into master Oct 6, 2022
@mikaelarguedas mikaelarguedas deleted the flake8_from_apt branch October 6, 2022 06:39
Guillaumebeuzeboc added a commit to ubuntu-robotics/nodl that referenced this pull request Oct 6, 2022
artivis pushed a commit to ubuntu-robotics/nodl that referenced this pull request Oct 12, 2022
* fix(CI): allow manual CI trigger
* fix(CI): update & upgrade before rosdep
* fix(CI): remove pip3 libraries
* fix(CI): macos, use updated ros action
* fix(CI): macOS, build ROS 2 from source
* fix(ci): docker image is now fixed
osrf/docker_images#641
* fix(CI): apply suggested changes
python3-flake8-deprecated \
python3-flake8-docstrings \
python3-flake8-import-order \
python3-flake8-quotes \
Copy link

@nachovizzo nachovizzo Nov 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mikaelarguedas , @ruffsl Sorry to jump out of nowhere here. But few doubts about the linter flake8-quotes. For the moment I will drop it here in case someone else out there is looking into this as well, soon will open a discussion in the forums

  1. Is there any good reason why we are using it!?
  2. Where can I find the original motivation for adding this?
  3. Where can I propose to completely get rid of such linter 🤓 ?

Thanks in advance and sorry for the annoyance!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @nachovizzo,

This is not something that is decided on the docker images maintaining side.
This is part of the ROS 2 styleguide: https://docs.ros.org/en/humble/The-ROS2-Project/Contributing/Code-Style-Language-Versions.html?highlight=style%20guide#id4 specifically this line:
"We pick single quotes over double quotes as long as no escaping is necessary."

I think the topic came up a couple times, I remember a thread on discourse a while back to compatible with other linters e.g. black https://discourse.ros.org/t/python-black-formatting/13277

Not sur if this is a satisfactory answer but that's a start :)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mikaelarguedas 🙌 Thanks! This is what I was missing , this entry point. Thanks a lot

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.

ROS 2 nightly seems to embed wrong version of flake8

3 participants