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

fixed simple deprecation warnings (issue #61) #63

Merged
merged 5 commits into from
Apr 28, 2020
Merged

fixed simple deprecation warnings (issue #61) #63

merged 5 commits into from
Apr 28, 2020

Conversation

zmk5
Copy link
Contributor

@zmk5 zmk5 commented Apr 27, 2020

Fixed two simple deprecation warnings as outlined in #61. The main deprecation warning, however, requires a bit more work since they have an issue with the yield function used within the async def.

@zmk5
Copy link
Contributor Author

zmk5 commented Apr 27, 2020

The build failure is because of the python 2.7 build.

Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

This looks generally good to me; thanks for the update.

@wjwwood Do we still care about Python 2 compatibility in this repository? If no, then I'll follow up with a PR disabling it in travis. If yes, we may have to do a bit more work here to make it compatible with both Python 2 and Python 3.

@wjwwood
Copy link
Member

wjwwood commented Apr 28, 2020

I'm fine with dropping Python 2 support now, but we have to stop release on Xenial at the same time.

@clalancette
Copy link
Contributor

I'm fine with dropping Python 2 support now, but we have to stop release on Xenial at the same time.

Out of curiousity, why? Xenial has python 3.5, which should be new enough to support the code here. (I don't have a strong opinion on it, I'm just wondering)

@wjwwood
Copy link
Member

wjwwood commented Apr 28, 2020

It is used by ROS 1 users in Xenial, and they use it with python2. We could instead only release new versions on Xenial as python3, leaving the python2 version behind, but that's a bit confusing that the python3 version suddenly is higher than python2.

@zmk5
Copy link
Contributor Author

zmk5 commented Apr 28, 2020

I added conditionals to the deprecation so once xenial is dropped we can remove them. I also found out getfullargspec() now returns 7 values instead of 4 according to the docs.

@wjwwood wjwwood merged commit ae04fa1 into osrf:master Apr 28, 2020
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.

3 participants