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

Abstract methods in non-abstract imporlib.metadata.Distribution #419

Closed
busywhitespace opened this issue Dec 23, 2022 · 6 comments · Fixed by #420
Closed

Abstract methods in non-abstract imporlib.metadata.Distribution #419

busywhitespace opened this issue Dec 23, 2022 · 6 comments · Fixed by #420
Assignees

Comments

@busywhitespace
Copy link

busywhitespace commented Dec 23, 2022

The empty methods read_text and locatate_file of Distribution class are decorated with abc.abstractmethod. But Distribution class doesn't have metaclass=abc.ABCMeta.

PEP 3119, Introducing Abstract Base Classes, states.:

The abstractmethod decorator should only be used inside a class body, and only for classes whose metaclass is (derived from) ABCMeta.

Considering the facts that methods read_text and locate_file are used in other methods of Distribution class and both are empty, only things containing are docstrings, I think the author of the class intended to use the abstractmethod functionality to enforce an implementation of the aforementioned methods. So, in my opinion, the Distribution class should have metaclass=abc.ABCMeta.

(Other possibility is that the aforementioned methods are intended as stubs. Then the abstractmethod decorator may be deleted.)

I've created the PR in case the issue is right, i.e. the class should contain metaclass=abc.ABCMeta.

Linked PRs

@jaraco
Copy link
Member

jaraco commented Dec 24, 2022

I can't remember if there was a reason why I chose not to configure the metaclass. Perhaps I thought it was optional. Thanks for the reference to the documentation, as it clarifies that ABCMeta should be used unconditionally if abstract methods are present.

Also, thanks for the PR, which illustrates that simply configuring the meta class isn't viable, because downstream consumers are depending on the non-enforcement of the interface, making introduction of the enforcement a backward-incompatible change.

I'll have to think about it more and determine if there's a path forward here and if so how. I'd like the test suite to include a test capturing the enhanced expectation here. It's also likely that this issue should be addressed first in importlib_metadata, which can iterate faster and get feedback on any approach sooner.

But before we embark on a solution, what is the importance of this issue? Is the goal mainly to align best practices or do you have a more concrete goal in mind?

@busywhitespace
Copy link
Author

I have encountered the issue when I was studying importlib.metatdata. So for me personally, I was just little bit confused by methods having abstractmethod, but the class not having metaclass=abc.ABCMeta.

So only goal was to clarify the confusion, why there are abstract decorated methods where the decorater has no effect.

@AlexWaygood
Copy link
Member

AlexWaygood commented Dec 25, 2022

why there are abstract decorated methods where the decorater has no effect.

This can be useful for type-checking purposes. While at runtime, the decorator only has an effect if the metaclass is ABCMeta or a subclass of ABCMeta, mypy respects the @abstractmethod decorator even if the metaclass is not ABCMeta or a subclass of ABCMeta. (That's a mypy feature, not a mypy bug, FYI.)

Decorating methods with @abstractmethod can also serve as useful documentation about the methods' intended purpose, even if the metaclass is not set.

@busywhitespace
Copy link
Author

Thank @AlexWaygood for the information about mypy checking @abstractmethod decorated methods.

If this use of the decorator is a good practice, then I am happy with the resolution.

@jaraco
Copy link
Member

jaraco commented Jan 1, 2023

In pypa/pip#11684, I reported the issue to pip so the usage can be corrected there, paving the way to potentially introduce the ABCMeta. In the meantime, it's probably safe to add the change to importlib_metadata to see if there are potentially other uses out there (seems unlikely). I'll transfer this issue to that project.

@jaraco
Copy link
Member

jaraco commented Apr 13, 2023

See #422 where I'm pondering how to handle the backward compatibility concerns this change revealed.

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 a pull request may close this issue.

3 participants