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

Import ABC from collections.abc for py 3.10 compatibility #5007

Merged
merged 6 commits into from
Mar 13, 2021

Conversation

tirkarthi
Copy link
Contributor

@tirkarthi tirkarthi commented Jul 24, 2020

Closes #5006

@pull-request-size pull-request-size bot added the size/S PR that changes 10-29 lines. Very easy to review. label Jul 24, 2020
@CLAassistant
Copy link

CLAassistant commented Jul 24, 2020

CLA assistant check
All committers have signed the CLA.

@arm4b
Copy link
Member

arm4b commented Jul 24, 2020

Looks like the CI is not happy as this change breaks the platform functionality and both py27 + py36 CI builds in many ways.
Take a look at CircleCI and TravisCI build failures for more context.

@tirkarthi
Copy link
Contributor Author

This was due to a typo where Mutableset has to be MutableSet (Capital S in Set). I have fixed it now.

@tirkarthi
Copy link
Contributor Author

The lint error doesn't make sense since the module name is moves and not Moves with capital M.

Using config file /home/travis/build/StackStorm/st2/lint-configs/python/.pylintrc
************* Module st2client.utils.types
E: 20, 0: No name 'collections_abc' in module 'Moves' (no-name-in-module)
Makefile:289: recipe for target '.pylint' failed

@cognifloyd
Copy link
Member

We just reformatted the code with black. So, would you rebase your branch on master (resolve any conflicts & reformat with black)? This should help resolve any linting errors.

Also, code on the master branch currently targets python 3.6+. We do not support python2.7 anymore, so that should simplify the changes needed here.

Thanks and Cheers!

@tirkarthi
Copy link
Contributor Author

Thanks, I have rebased and resolved conflicts. I have used collections.abc instead of six.moves.collections_abc since the repo is Python 3.6 + only. The lint stage passes now. This change was implemented in Python 3.10 and will become an error : python/cpython#23754

Copy link
Member

@cognifloyd cognifloyd left a comment

Choose a reason for hiding this comment

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

LGTM
A maintainer will need to approve and merge it.

@tirkarthi
Copy link
Contributor Author

Thanks @cognifloyd

@arm4b
Copy link
Member

arm4b commented Mar 8, 2021

Thanks, @cognifloyd for the heads up.
@tirkarthi can you please add a Changelog record for this enhancement and so we could merge it?

@arm4b arm4b added this to the 3.5.0 milestone Mar 8, 2021
Copy link
Member

@arm4b arm4b left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks for the contribution, @tirkarthi and @cognifloyd for review!

@arm4b arm4b changed the title Import ABC from collections.abc Import ABC from collections.abc for py 3.10 compatibility Mar 13, 2021
@arm4b arm4b merged commit 6753951 into StackStorm:master Mar 13, 2021
@tirkarthi
Copy link
Contributor Author

Thanks all for the review and merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement size/S PR that changes 10-29 lines. Very easy to review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Importing ABC directly from collections has been deprecated and will be removed in Python 3.10
6 participants