-
-
Notifications
You must be signed in to change notification settings - Fork 82
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
version 6.0.0 change of making Distribution an abstract class breaks some projects #422
Comments
This change also breaks rinohtype: brechtm/rinohtype#389. Things seems to work fine with the stdlib's importlib.metadata. |
I somehow managed to miss this report until just now. I'll take a look at this issue soon. |
Thanks both for your understanding while we try to make the implementation more correct. This change was requested to be added to CPython as well, to land in Python 3.12, and I need to decide how dangerous this change is. The options as I see it are:
IncorporateSince the projects that were impacted have addressed the compatibility, I'm tempted to say it should be safe to introduce the incompatibility into the stdlib as well. That will have the effect of making these libraries (rinohtype, logilab, pytest-capture-deprecated-warnings) non-viable for newer Pythons and older releases of the libraries. Deprecation PeriodThe second option is a lot of work with very little benefit (now that affected projects have adapted). Roll backThis third option restores the prior expectation and re-opens the opportunity for improper behavior, leaving the code in a somewhat inconsistent state. Drop ABCThe fourth option is somewhat attractive, given that such behavior was apparently desirable in at least three instances. Since the spec for |
I tried to look into these projects to see their source, but I couldn't find either of them. I did find logilab-common on PyPI, but the links to the project sources are broken. I see now that the issue is that one shouldn't be constructing |
@brechtm I appreciate the toil this change has caused by not rolling back the change more promptly, so I'm sensitive to asking you for more input, but I'd really like your opinion - what would you like to see from this package before applying the change to Python 3.12? I'm leaning toward the first or last options. If we chose the first option, rinoh would need to continue to implement the not-implemented methods. If we go with the last option, rinoh would at some point be able to drop those concrete not-implemented methods. |
After mulling it overnight, I'm now feeling stronger toward the Drop ABC option, because there is at least one user (rinoh) that expects not to have to implement those methods. |
After drafting the implementation in #448, I'm less confident in that option. I'm continuing to waffle between that and just pushing the existing behavior upstream to stdlib. I guess the real question is a strategic one - should Distribution be lenient to a partial implementation, or should it require subclasses to explicitly acknowledge their partial implementation by supplying NotImplemented methods? When I phrase it that way, I do think it's better to structure it such that it's more constrained by default, because most subclasses are expected to need to implement these methods. Let's try incorporating the 6.0 changes into Python 3.12 and see if that reveals any additional concerns or if we can keep the stricter implementation. |
Thanks for asking for input! I don't have a strong opinion about this specific issue since this is handled in rinohtype now (but not yet in the latest release, I think). Assuming you're using symantic versioning, I can't really complain about 6.0.0 being backwards-incompatible, either. In general, I am a bit worried about maintaining compatibility with the different versions of importlib.metadata in the Python versions I want to support (all that haven't reached EOL) and all of the versions of importlib-metadata. I want to avoid setting version constraints as that might cause conflicts with other packages that do (e.g. Sphinx). To help with this, it would be good if future versions of importlib-metadata maintain backward compatibility as much as possible. In this case, that means making Distribution allow a partial implementation. |
In python/cpython#103584 (comment), I see that pip is also affected, indicating that option 1 may not be viable, so now I'm leaning toward option 2. |
In #451, I've drafted an implementation of option 2. |
Hello,
As stated in the changelog:
This change indeed breaks at least 2 projects (logilab.common and pytest-capture-deprecated-warnings). Both those projects are using importlib_metadata this way:
(this might not be the best way to use this lib)
While not having to use the 2 required methods:
Those projects aren't very big in the python ecosystem and I'm the process of fixing them (by creating a subclass with the required methods) but since the changelog is asking for a report here it is.
Cheers and thx for your work ❤️
The text was updated successfully, but these errors were encountered: