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

[MRG] Update hide_modules.py #852

Merged
merged 5 commits into from
Jul 20, 2023
Merged

[MRG] Update hide_modules.py #852

merged 5 commits into from
Jul 20, 2023

Conversation

alcir
Copy link
Contributor

@alcir alcir commented Jul 20, 2023

Remove deprecated importlib.abc.Finder

Reference issue

#851

Remove deprecated importlib.abc.Finder
Comment on lines 30 to 36
import importlib.abc

# py>=3.3 has MetaPathFinder
_ModuleHiderBase = getattr(importlib.abc, "MetaPathFinder", importlib.abc.Finder)
_ModuleHiderBase = getattr(importlib.abc, "MetaPathFinder")
except ImportError:
# py2
_ModuleHiderBase = object
Copy link
Member

@mrbean-bremen mrbean-bremen Jul 20, 2023

Choose a reason for hiding this comment

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

While at it, you can remove this try/except, which was only needed for Python 2 compatibility, and directly access the attribute instead of using getattr.

It would also make sense to add tests for Python 3.12-dev (and 3.11, for that matter) in merge-pytest.yaml to see this kind of problems in the future.

@codecov
Copy link

codecov bot commented Jul 20, 2023

Codecov Report

Merging #852 (5471493) into master (a5beba7) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##            master      #852   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           28        28           
  Lines         8639      8639           
=========================================
  Hits          8639      8639           

Remove try catch for Python 2
import importlib.abc

# py>=3.3 has MetaPathFinder
_ModuleHiderBase = getattr(importlib.abc, "MetaPathFinder")
Copy link
Member

@mrbean-bremen mrbean-bremen Jul 20, 2023

Choose a reason for hiding this comment

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

Why still use getattr? Actually, this variable should not be needed anymore, just use

from importlib.abc import MetaPathFinder

class ModuleHider(MetaPathFinder):
    ...

class ModuleHider(MetaPathFinder):


class ModuleHider(_ModuleHiderBase):
class ModuleHider(MetaPathFinder)::
Copy link
Member

Choose a reason for hiding this comment

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

Typo - double colon.
Can you also please add 3.11 and 3.12-dev to the CI tests as mentioned above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Uh, how to add 3.11 and 3.12-dev to the CI tests?
Sorry but I'm not very well versed to development, git and so on

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I can just do it myself if you don't mind.

Copy link
Member

Choose a reason for hiding this comment

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

Just did it - I hope you don't mind...

@penguinpee
Copy link

I prefer the approach in #853, wrapping getattr in try except. But as long as it works I'm fine with either.

@mrbean-bremen
Copy link
Member

I prefer the approach in #853, wrapping getattr in try except. But as long as it works I'm fine with either.

Why would you prefer getattr to the direct attribute access? This has only been a workaround for the Python2 incompatibility. Also, linters cannot check getattr calls.

And I closed your PR only because it was a duplicate, not because I did not like it :)

@penguinpee
Copy link

Why would you prefer getattr to the direct attribute access? This has only been a workaround for the Python2 incompatibility. Also, linters cannot check getattr calls.

Well, getattr has already been used. Apparently that worked up until python3.12. Now it throws an AttributeError. Apparently something has changed in importlib.abc. I was simply catching that exception, leaving versions < 3.12 untouched.

Of course that could be done without getattr.

And I closed your PR only because it was a duplicate, not because I did not like it :)

No hard feelings. I should have looked better before submitting my PR. I'm not asking you to use it. It's up to you to decide on the final fix. Removing leftover cruft from 2.x ages is probably a good idea.

With the simple(r) patch from #853 everything is working again in Fedora. Next release we'll take whatever you provide. 😉

@mrbean-bremen
Copy link
Member

Well, getattr has already been used. Apparently that worked up until python3.12.

Thats true, but that usage has been deprecated for quite some time, and using getattr prevented to show the deprecation warning.
Anyway, you are right - the main thing is that it works...

- fix syntax in last change
@mrbean-bremen
Copy link
Member

I'm going to merge now - some test is randomly failing, this is handled in another issue.

@mrbean-bremen mrbean-bremen merged commit 1511488 into pydicom:master Jul 20, 2023
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