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

Abc meta classes #96

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Abc meta classes #96

wants to merge 2 commits into from

Conversation

jlvdb
Copy link
Contributor

@jlvdb jlvdb commented Aug 3, 2023

This pull requests adds abc.ABC as meta classes for Pipeline and PipelineStage.

Both classes implement a few abstract methods, however they do not use abc.ABC as meta class, which would prevent to instantiate a subclass if it does not properly implement the abstract methods. Currently a NotImplementedError is raised at a later point during runtime.

@jlvdb
Copy link
Contributor Author

jlvdb commented Aug 3, 2023

What is the purpose of the abstract method here? Clearly the issue I tried to address is a design feature, but I was not able to figure it out.

@joezuntz
Copy link
Collaborator

joezuntz commented Aug 3, 2023

Hi @jlvdb - thanks for looking at this! I'm not quite clear what you mean by your question - could you explain more?

@jlvdb
Copy link
Contributor Author

jlvdb commented Aug 4, 2023

Hi @joezuntz, I did not really understand this line here:

and not getattr(cls.run, "__isabstractmethod__", False)

Since the it checks for isabstractmethod, I thought that the @AbstractMethod serves a purpose beyond indicating an abstract method that should be overwritten, since suddenly the unit tests were failing.

@joezuntz
Copy link
Collaborator

joezuntz commented Aug 4, 2023

I don't really remember the details here, but I think the main reason I didn't go full-on with using the ABC superclass was that the error messages were much less informative for a typical user. So I guess I was manually using this test to make sure that the user overrode stuff so I could give a more useful custom error message.

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.

2 participants