Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add python_version key to REP 153 #207

Merged
merged 3 commits into from Oct 7, 2019

Conversation

@dirk-thomas
Copy link
Member

commented Oct 1, 2019

This specifies the semantic of the new key python_version introduced in ros-infrastructure/rosdistro#145.

@dirk-thomas dirk-thomas self-assigned this Oct 1, 2019
rep-0153.rst Outdated Show resolved Hide resolved
@dirk-thomas dirk-thomas force-pushed the dirk-thomas/add-python_version-to-index branch from 18d67f9 to 995d6d6 Oct 1, 2019
Copy link
Contributor

left a comment

LGTM. I've left a couple wording suggestions below.

rep-0153.rst Outdated Show resolved Hide resolved
rep-0153.rst Outdated Show resolved Hide resolved
dirk-thomas and others added 2 commits Oct 2, 2019
Co-Authored-By: Jacob Perron <jacob@openrobotics.org>
Co-Authored-By: Jacob Perron <jacob@openrobotics.org>
@tfoote
tfoote approved these changes Oct 3, 2019
* python_version: an optional integer describing the major version of
Python of the ROS distribution.
Comment for lines +81  – +82

This comment has been minimized.

Copy link
@kyrofa

kyrofa Oct 3, 2019

Given that this is optional, should the REP define a default?

This comment has been minimized.

Copy link
@dirk-thomas

dirk-thomas Oct 3, 2019

Author Member

I don't think there is any default value which would be reasonable / not have common cases where it would be wrong. The official rosdistro database is being update to define the value for every ROS distro: ros/rosdistro#22484.

This comment has been minimized.

Copy link
@kyrofa

kyrofa Oct 3, 2019

Well, given that the REP specifies that unknown keys are to be ignored, along with the fact that until very recently everything was python 2, clients will be assuming python 2 unless support for this key is added, right? In other words, it seems like the de-facto default is 2, here, but only because of assumptions made by consumers of the REP. That seems potentially problematic, but I suppose also mostly academic.

This comment has been minimized.

Copy link
@dirk-thomas

dirk-thomas Oct 3, 2019

Author Member

along with the fact that until very recently everything was python 2, clients will be assuming python 2 unless support for this key is added, right?

That is incorrect. While all ROS 1 distros up to Melodic are using Python 2 all ROS 2 distributions are using Python 3.

This comment has been minimized.

Copy link
@kyrofa

kyrofa Oct 3, 2019

Great point-- that distinction is currently hard-coded then, yes? But it's "easy" right now because you can split ROS 1->Python 2, ROS 2->Python 3, until Noetic. Which comes back to this PR. Very good, thank you.

@kyrofa
kyrofa approved these changes Oct 3, 2019
@dirk-thomas dirk-thomas merged commit 5635c7c into master Oct 7, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@dirk-thomas dirk-thomas deleted the dirk-thomas/add-python_version-to-index branch Oct 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.