On Tue, Apr 23, 2024 at 12:58 PM Luca Vizzarro <luca.vizza...@arm.com> wrote: > > Hi Juraj! > > I am not fussed about naming but I am certainly in favour of making > things less confusing, so I am happy with this change. >
Yes, using execution in this context is very confusing. The main thing with this patch is to find the best replacement. If you're ok with the new name or can't think of a (even potentially) better name, that's all we need. :-) > On 19/04/2024 09:51, Juraj Linkeš wrote: > > @staticmethod > > def from_dict( > > - d: ExecutionConfigDict, > > + d: TestRunConfigDict, > > node_map: dict[str, Union[SutNodeConfiguration | > > TGNodeConfiguration]], > > - ) -> "ExecutionConfiguration": > > + ) -> "TestRunConfiguration": > > <snip> > > - def copy_and_modify(self, **kwargs) -> "ExecutionConfiguration": > > + def copy_and_modify(self, **kwargs) -> "TestRunConfiguration": > > <snip> > > @staticmethod > > def from_dict(d: ConfigurationDict) -> "Configuration": > > Unrelated, but spark for an improvement. I have noticed these, not sure > if there are more instances in the code. Shouldn't from_dict be a > @classmethod instead of @staticmethod. In which case you could also use: > > from typing_extensions import Self > > to replace all the class literals, example: > > @classmethod > def from_dict(cls, d, node_map): -> Self: > ... > return cls(...) # initialise class with cls > > ... > > def copy_and_modify(self, **kwargs) -> Self: There are many of these from_dict methods. Maybe we should just put the logic into __init__ and not use the constructors generated by the dataclass decorator. Or we could also use your suggestion. I don't know that they should be class methods, it doesn't make that much of a difference (is there an appreciable one? Apart from the nicer typing). It's worth making a bugzilla ticket for this though.
