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

Rely on 'selectable' interface for entry points. #2126

Merged
merged 11 commits into from
Jul 9, 2021
Merged

Conversation

jaraco
Copy link
Member

@jaraco jaraco commented May 29, 2021

@jaraco jaraco marked this pull request as draft May 29, 2021 08:59
setup.cfg Outdated Show resolved Hide resolved
setup.cfg Outdated Show resolved Hide resolved
@jaraco jaraco force-pushed the bugfix/2093-new-api branch from efb2d30 to e294994 Compare May 29, 2021 09:41
@codecov
Copy link

codecov bot commented May 29, 2021

Codecov Report

Merging #2126 (aad8790) into main (d255a23) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2126      +/-   ##
==========================================
- Coverage   93.70%   93.67%   -0.03%     
==========================================
  Files          88       88              
  Lines        4385     4382       -3     
==========================================
- Hits         4109     4105       -4     
- Misses        276      277       +1     
Flag Coverage Δ
tests 93.67% <100.00%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/virtualenv/run/plugin/base.py 97.50% <100.00%> (-0.18%) ⬇️
src/virtualenv/seed/embed/base_embed.py 96.22% <0.00%> (-1.89%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d255a23...aad8790. Read the comment docs.

@jaraco jaraco marked this pull request as ready for review May 29, 2021 15:00
Copy link
Contributor

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

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

@jaraco you'll need to fix the failing CI:

@github-actions check / test pypy3 - windows-latest (push) 

@gaborbernat
Copy link
Contributor

@jaraco please in future work of your fork, and not create branches in this repo; thanks!

@gaborbernat gaborbernat merged commit 17caadc into main Jul 9, 2021
@gaborbernat gaborbernat deleted the bugfix/2093-new-api branch July 9, 2021 15:30
@hroncok
Copy link
Contributor

hroncok commented Dec 2, 2021

Would you mind if we sent a PR that conditionalizes this for Python < 3.10?

@jaraco
Copy link
Member Author

jaraco commented Dec 3, 2021

Would you mind if we sent a PR that conditionalizes this for Python < 3.10?

I can't speak to virtualenv's position on this request, but I'd like to know more about the motivation. Is the new dependency an issue? backports.entry_points_selectable allows for vendoring, so if the dependency is the issue, I'd recommend to consider vendoring the backport. The backport was developed to help avoid each consumer having to write the fallback logic and give each consumer a straightforward single-line import (until dropping support for Python 3.9).

@hroncok
Copy link
Contributor

hroncok commented Dec 3, 2021

We have a natural tendency to avoid packaging backports for Python versions that contain the backported code. The Fedora's virtualenv has this patched out entirely on Python 3.10+ and I was wondering if we can drop that difference by doing that here instead.

No, bundling is even worse.

@gaborbernat
Copy link
Contributor

@hroncok so #2238 would solve your issue?

@hroncok
Copy link
Contributor

hroncok commented Dec 3, 2021

@hroncok so #2238 would solve your issue?

Totally, that is the exact change I had in mind. Sorry for not looking for it first.

@gaborbernat
Copy link
Contributor

I will accept that, I'm not that keen on the backports.entry_points_selectable design, perhaps we can drop it entirely for the earlier python versions too.

@jaraco
Copy link
Member Author

jaraco commented Dec 3, 2021

I suspect that will address the issue. The question is - should hroncok go project by project requesting this change in contradiction to the recommendation of the backport (https://github.com/jaraco/backports.entry_points_selectable/blame/main/README.rst#L29), or should the backport provide the recommendation and the special incantation for everyone to adopt? I'd prefer to provide the best practices at a centralized place rather than leave it to each project to figure out their localized best practice and leave it to each project to reconcile from a variety of different practices.

The main reason I wrote the backport was to provide a one-line solution for each project, but this recommendation is a multi-line implementation that isn't even readily implementable as a cohesive block of code. (import sys and try/except blocks). I'm trying to make it clean and easy for adopters and the proposed change diverges from that.

We have a natural tendency to avoid packaging backports for Python versions that contain the backported code.

If you think there's a better way that projects could/should adopt selectable entry points, I'd welcome your feedback at either importlib_metadata or backports.entry_points_selectable.

@hroncok
Copy link
Contributor

hroncok commented Dec 3, 2021

I don't see anything wrong in using backports only where needed. Yes, it is a bit more lines of code, but it clearly shows when it can be deleted. I don't think I know all the best practices of the world, I only cared about virtualenv now. I wanted to avoid a superfluous dependency which would only mean more work for us. If virtualenv would not agree, I would not try to force it in any way.

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.

DeprecationWarning with importlib-metadata>=3.9.0
3 participants