Drop step-specific "base Click command" implementations#4457
Drop step-specific "base Click command" implementations#4457
Conversation
8af29bc to
b5373c7
Compare
LecrisUT
left a comment
There was a problem hiding this comment.
LGTM, just 2 minor issues.
|
|
||
|
|
||
| # Establish the "plugin class -> step class" link. | ||
| CleanupPlugin._step_class = Cleanup |
There was a problem hiding this comment.
Why not put these inside the class itself?
There was a problem hiding this comment.
When CleanupPlugin is defined, there is no Cleanup class yet. When Cleanup is defined, CleanupPlugin is defined too, and Cleanup can include the "link":
class CleanupPlugin(tmt.steps.Plugin[CleanupStepDataT, PluginOutcome]):
...
class Cleanup(tmt.steps.Step):
...
_plugin_base_class = CleanupPlugin
...I tried to explain that in https://github.com/teemtee/tmt/pull/4457/changes#diff-d9422da1a3efb9209a7d4beb7d824f85a19e5abe91fb239f74806847d88db266R1551, and I don't see any way how to overcome this circular linking with plain class-level attributes, assigned a type. _step_class = Cleanup would fail on Cleanup not existing yet. Class A needs to point at class B, and class B needs to point at class A, in this case A needs to wait for B to be completed and initialize its link after that.
There was a problem hiding this comment.
__future__.annotations should be able to use this. Otherwise fair enough, can't think of other ways to workaround that circular definition
There was a problem hiding this comment.
Annotations wouldn't be a problem, it's all killed by the assignment.
There was a problem hiding this comment.
Ah right, that doesn't work. Don't think any construct could help us here unless we can postpone the class definition at the end of the module read. Only other way would be to make it an abstract class method I guess.
There was a problem hiding this comment.
I had an abstract class method, it worked, but it turned out to be a lot of boilerplate just for one assignment. Eventually, I decided to go this way, also thinking about dropping the assignment from the step class, i.e. _plugin_base_class = CleanupPlugin, and do it explicitly, after both classes are created. That would put this "establish the links between classes" matter at one place, both directions - right now it's split between the step class and the post-step class assignment.
| #: When the plugin base class is defined, the step class does not | ||
| #: even exist yet. Therefore this link is set after both classes, | ||
| #: step and its plugin base, are finalized. | ||
| _step_class: type[Step] |
There was a problem hiding this comment.
Can we ClassVar this? Probably also Final or something. There is some abstract-like support to make sure it is defined, but can't remember what typing was doing it.
PS: I think attrs could extract this from the generic.
There was a problem hiding this comment.
Can we
ClassVarthis? Probably alsoFinalor something. There is some abstract-like support to make sure it is defined, but can't remember what typing was doing it.
ClassVar shouldn't be a problem. Final might be, we do assign it at least once, but we could silence that occasion and report every other attempt.
PS: I think attrs could extract this from the generic.
A type checker can for sure, if we add the step class annotation among into Generic, we could use it here:
Generic[StepDataT, PluginReturnValueT, StepT]
_step_class: type[StepT]It goes a bit too far beyond the scope of this PR, but in general it's something I'd like to see, as plugins should restrict the type of the step they belong to.
They are not that different, each step class just used different step name, and that was it. Dropping the `base_command` method from `Step`-like classes, replacing them with a shared implementation. Also, `--how` now prints allowed/known plugins as possible choices. I was editing the help text, and this felt like a natural change of the option.
b5373c7 to
d9fc940
Compare
They are not that different, each step class just used different step name, and that was it. Dropping the
base_commandmethod fromStep-like classes, replacing them with a shared implementation.Also,
--hownow prints allowed/known plugins as possible choices. I was editing the help text, and this felt like a natural change of the option.Pull Request Checklist