search

LSSTDESC / ceci

Experimental pipeline prototype software
BSD 3-Clause "New" or "Revised" License
13 stars 9 forks source link

Abc meta classes #96

Open jlvdb opened 1 year ago

jlvdb commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

jlvdb commented 1 year ago

Hi @joezuntz, I did not really understand this line here: https://github.com/LSSTDESC/ceci/blob/5c683f8fd6a8bdd2b36c1f6f03f4d24599307a54/ceci/stage.py#L293 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 commented 1 year ago

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.