E003a restructure#441
Conversation
58d6912 to
4d93362
Compare
4d93362 to
3da0c6b
Compare
|
Please rebase. |
3da0c6b to
d494b90
Compare
d494b90 to
933199b
Compare
|
|
||
| simargs[parts[0]] = override_dict | ||
|
|
||
| return simargs |
There was a problem hiding this comment.
The classes names sounds a bit confusing to me.
ModelExecutionData (currently a dataclass) actually does things — it has get_cmd() and run() methods. It's the active executor.
ModelExecutionCmd (currently a regular class) is purely a builder/configurator — it accumulates arguments via arg_set(), args_set(), arg_get(), and ultimately produces a ModelExecutionData instance via definition(). It holds no executable logic itself.
- Something called
...Datashould be a passive data holder (which is exactly what a@dataclassnormally is), and - Something called
...Cmdsuggests an active command runner.
But right now it's the opposite: the @dataclass runs the process, and the class called Cmd just builds up configuration.
If you want to make the roles clearer without changing much, something like ModelExecutionConfig (for the builder that accumulates args) and ModelExecutionRun (for the dataclass that actually executes) would remove the ambiguity entirely rather than relying on Cmd vs Data to carry the distinction.
What do you think?
There was a problem hiding this comment.
I'm fine with the renames; would it be OK to do this after the restructure (E003a to E003c)? Else it will quite some work on modifying the data again and again ... I could create a PR for this on top of E003c or F001.
The restructure just move classes and adapt imports, etc. - no change to the content of any class!
Background (as you stated; via a long way of development):
- the
dataclasscollects all the data and has the (minimal) function to execute it - the other class is just responsible to build / create all the needed variables of the
dataclass
There was a problem hiding this comment.
Sure, I will merge this and you can put the change on top.
restructure - part1: collect all ModelExecution classes in one file