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

Switching to importlib resources #4056

Merged
merged 10 commits into from
May 25, 2023
Merged

Switching to importlib resources #4056

merged 10 commits into from
May 25, 2023

Conversation

IAlibay
Copy link
Member

@IAlibay IAlibay commented Mar 6, 2023

Fixes #4055

Still some work to be done here (mostly getting rid of __all__ and transferring the information across), but I thought I'd get this in now and let folks comment early since this one is likely to be contentious.

Notes:

  • Because the old pkg_resources approach we took generated strings, we can't just use the Path objects directly, hence all the str calls (there's probably a nicer way to do this, I spent all of ~ 10 mins on this).

PR Checklist

  • Tests?
  • Docs?
  • CHANGELOG updated?
  • Issue raised/referenced?

@IAlibay IAlibay changed the title [wipImportlib resources [wip] Switching to importlib resources Mar 6, 2023
@IAlibay IAlibay changed the title [wip] Switching to importlib resources [WIP] Switching to importlib resources Mar 6, 2023
@mattwthompson
Copy link
Contributor

Following out of self-interest as I need to make parallel changes in a few places

hence all the str calls

There's also Path.as_posix() which I use because it feels like it might be safer, but I can't find any evidence of that. It's been synonymous with str(Path) in every case I've run into on Unix-like systems ... not sure how much Windows support complicates things as I don't make an effort to do that.

https://docs.python.org/3/library/pathlib.html#pathlib.PurePath.as_posix

@IAlibay
Copy link
Member Author

IAlibay commented Mar 14, 2023

So this code won't work with py3.8. There are ways to make it backwards compatible with py3.8, but honestly given that NEP29 means we ditch py3.8 on the 14th of April, I would suggest just pushing the 2.5.0 release until then and using the modern API for dealing with resources?

@IAlibay IAlibay mentioned this pull request Mar 15, 2023
4 tasks
@IAlibay IAlibay added this to the 2.5.0 milestone Apr 12, 2023
@github-actions
Copy link

github-actions bot commented Apr 20, 2023

Linter Bot Results:

Hi @IAlibay! Thanks for making this PR. We linted your code and found the following:

Some issues were found with the formatting of your code.

Code Location Outcome
main package ✅ Passed
testsuite ⚠️ Possible failure

Please have a look at the darker-main-code and darker-test-code steps here for more details: https://github.com/MDAnalysis/mdanalysis/actions/runs/5065655276/jobs/9094482219


Please note: The black linter is purely informational, you can safely ignore these outcomes if there are no flake8 failures!

@codecov
Copy link

codecov bot commented May 24, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (374f0e9) 93.61% compared to head (b02b4f0) 93.61%.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #4056   +/-   ##
========================================
  Coverage    93.61%   93.61%           
========================================
  Files          192      192           
  Lines        25146    25146           
  Branches      4058     4058           
========================================
  Hits         23540    23540           
  Misses        1089     1089           
  Partials       517      517           

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@IAlibay IAlibay changed the title [WIP] Switching to importlib resources Switching to importlib resources May 24, 2023
@IAlibay
Copy link
Member Author

IAlibay commented May 24, 2023

Spurious codecov failure aside - this seems to be working fine now.

@MDAnalysis/coredevs can I get a couple of extra eyes on a final review here please? This is critical for 2.5.0.

Copy link
Member

@orbeckst orbeckst left a comment

Choose a reason for hiding this comment

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

Thanks to you and sed!

@IAlibay IAlibay merged commit 5ab95ec into develop May 25, 2023
@IAlibay IAlibay deleted the importlib_resources branch May 25, 2023 08:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move away from pkg_resources to importlib
4 participants