Create more elaborate base class for curation plugins #441
Conversation
src/hermes/commands/curate/accept.py
Outdated
|
|
||
| def process_decision_positive(self): | ||
| """In case of positive curation result, copy files to next step.""" | ||
| ctx = CodeMetaContext() |
There was a problem hiding this comment.
Why a new CodeMetaContext() and not self.ctx? Does that work?
There was a problem hiding this comment.
It did work. But it was just moved over from the old code I replaced it with self.ctx in d8ce5f8 :-)
sdruskat
left a comment
sdruskat
left a comment
There was a problem hiding this comment.
Thanks! Some comments, of which only one is more than minor:
Should the default case be a plugin? It's an architectural (and project management) decision to be made. All plugins are meant to be extracted, should there be an exemption for default cases on the command level?
| [project.entry-points."hermes.curate"] | ||
| accept = "hermes.commands.curate.accept:AcceptCuratePlugin" |
There was a problem hiding this comment.
I don't think we've made a decision to let the default plugins for each command live in hermes, but that's fine for now (#360).
There was a problem hiding this comment.
You mean directly in hermes, i.e. accept = "hermes:AcceptCuratePlugin"?
There was a problem hiding this comment.
Oh, as a separate repository you mean. I also missed the first "don't" 🤦🏻♂️
| class AcceptCuratePlugin(BaseCuratePlugin): | ||
| """Accept plugin for the curation step. | ||
| This plugin creates a positive curation result, i.e. it accepts the produced | ||
| metadata as correct and lets the execution continue without human intervention. It | ||
| also copies the metadata produced in the process step to the "curate" directory. | ||
| """ |
There was a problem hiding this comment.
Why is this a plugin and not the default implementation in the base plugin?
There was a problem hiding this comment.
To me, this felt like the right way to do this. Accepting everything without check is one curation strategy, and this strategy is the default choice.
There was a problem hiding this comment.
I could make the base class an ABC to make this clearer.
| #: Parameter by which the plugin is selected. By default, the accept plugin is used. | ||
| method: str = "accept" |
There was a problem hiding this comment.
See above re plugin. method: str may be enough.
| class BaseCuratePlugin(HermesPlugin): | ||
| """Base class for curation plugins. | ||
| Objects of this class are callables. |
There was a problem hiding this comment.
Should this be documented in HermesPlugin (and in https://github.com/softwarepub/hermes/blob/develop/docs/source/tutorials/writing-a-plugin-for-hermes.md)?
There was a problem hiding this comment.
Yes, I'll move it 😄
| """ | ||
| pass | ||
|
|
||
| def get_decision(self) -> bool: |
There was a problem hiding this comment.
| def get_decision(self) -> bool: | |
| def is_publication_ready(self) -> bool: |
Better naming, alternatively is_publication_allowed, is_approved, or similar.
Co-authored-by: Stephan Druskat <sdruskat@users.noreply.github.com>
This pull request establishes a base class for curation plugins and implements the old dummy curation in terms of this class.
Closes #440