Skip to content

Abc meta classes#96

Open
jlvdb wants to merge 2 commits into
LSSTDESC:masterfrom
jlvdb:abc-meta-classes
Open

Abc meta classes#96
jlvdb wants to merge 2 commits into
LSSTDESC:masterfrom
jlvdb:abc-meta-classes

Conversation

@jlvdb

@jlvdb jlvdb commented Aug 3, 2023

Copy link
Copy Markdown
Contributor

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

jlvdb commented Aug 3, 2023

Copy link
Copy Markdown
Contributor Author

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

joezuntz commented Aug 3, 2023

Copy link
Copy Markdown
Collaborator

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

@jlvdb

jlvdb commented Aug 4, 2023

Copy link
Copy Markdown
Contributor Author

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

joezuntz commented Aug 4, 2023

Copy link
Copy Markdown
Collaborator

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