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

Changed importlib_metadata to have requirement for < python 3.10 #577

Merged
merged 10 commits into from Jun 5, 2022

Conversation

probro27
Copy link
Contributor

Trying to fix the issue #575.
Hope it works!!

Copy link
Collaborator

@mitya57 mitya57 left a comment

Choose a reason for hiding this comment

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

Also please see what I wrote in this comment:
#576 (review)

setup.cfg Outdated Show resolved Hide resolved
probro27 and others added 3 commits May 27, 2022 13:07
Co-authored-by: Dmitry Shachnev <mitya57@users.noreply.github.com>
@probro27
Copy link
Contributor Author

I have updated the imports as per #576 (review)
Hope this works!

tests/test_packaging.py Outdated Show resolved Hide resolved
Co-authored-by: Dmitry Shachnev <mitya57@users.noreply.github.com>
@mitya57 mitya57 requested a review from jaraco May 27, 2022 21:10
@probro27
Copy link
Contributor Author

Hmm, any idea what might cause the tests to fail? I didn't change anything that could cause errors. I'll try to investigate. If you find anything, let me know!
Thanks!

@mitya57
Copy link
Collaborator

mitya57 commented May 27, 2022

So mypy complains about this:

______________________________ keyring/backend.py ______________________________
15: error: Name "metadata" already defined (by an import)
___________________________ tests/test_packaging.py ____________________________
4: error: Name "metadata" already defined (by an import)

I would simply silence that. Try adding # type: ignore comments at the end of line 15 of keyring/backend.py and line 4 of tests/test_packaging.py.

Like this:

try:
    from importlib import metadata
except ImportError:
    import importlib_metadata as metadata  # type: ignore

@probro27
Copy link
Contributor Author

Alright I have added that. Thanks for the help!
Let's hope it works now!

@mitya57
Copy link
Collaborator

mitya57 commented May 28, 2022

I am sorry. It still doesn't work because Python 3.9 and earlier does have importlib.metadata available, although it does not support group argument, so we can't use it.

Let's try this:

import sys

if sys.version_info >= (3, 10):
    from importlib import metadata
else:
    import importlib_metadata as metadata

Add import sys

I hope # type: ignore won't be needed with this, but let's see.

@probro27
Copy link
Contributor Author

I have added the # type:ignore on the imports for now, coz I was getting a linting error when I was not doing that. I hope this works!

@probro27
Copy link
Contributor Author

_________________________________ FLAKE8-check _________________________________ /home/runner/work/keyring/keyring/tests/test_packaging.py:6:42: E261 at least two spaces before inline comment /home/runner/work/keyring/keyring/tests/test_packaging.py:6:43: E262 inline comment should start with '# '
Does this seem to be the error?

@mitya57
Copy link
Collaborator

mitya57 commented May 28, 2022

Yes, you need two spaces before # and one space after #. And please remove commented out code.

@probro27
Copy link
Contributor Author

Okay, I have changed the formatting and removed the comments.

Copy link
Collaborator

@mitya57 mitya57 left a comment

Choose a reason for hiding this comment

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

Yes, now CI finally passes. Thanks!

You have my approval, but let's wait for @jaraco who is the chief maintainer.

@jaraco jaraco linked an issue Jun 5, 2022 that may be closed by this pull request
Copy link
Owner

@jaraco jaraco left a comment

Choose a reason for hiding this comment

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

I added a couple of nitpicky changes and a changelog, but this looks good. Thanks for pulling it together!

@jaraco jaraco merged commit d106359 into jaraco:main Jun 5, 2022
@probro27
Copy link
Contributor Author

probro27 commented Jun 7, 2022

Thanks for merging it!
Glad to be of help :))

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.

Make importlib_metadata optional on Python 3.10.
3 participants