[PATCH v2 1/2] tools: Add script to create artifacts

Juraj Linkeš juraj.linkes at pantheon.tech
Mon Jan 15 14:07:05 CET 2024


> +    def set_properties(self):

I see that you've left properties as a dictionary. Looking at how it's
used, it makes sense.
I think we can improve this method. To understand what this method
sets, you'd need to look at the code (instead of having an idea when
it's called). It could still be named set_properties with keyword
arguments being what we're setting:

def set_properties(self, **kwargs):
    for key in kwargs:
        self.properties[key] = kwargs[key]

This way it's obvious what we're setting when calling set_properties:
self.set_properties(tree=self.tree, applied_commit_id=self.commit_id)

I like this more than renaming this to something like
set_tree_and_commit_id, as it's more flexible.

<snip>

> +def parse_args() -> CreateSeriesParameters:

This function name is misleading, as it's not just parsing the args
(and returning those), but also processing them and returning a
different object. I suggest either renaming this or splitting the
function into two (one that parses, the other that does the rest).

<snip>

> +def main() -> int:
> +    data = parse_args()

You can see here what I mean. I'd expect the parse_args to return
args, not data, so it's a bit confusing and requires investigation of
the parse_args method.


More information about the ci mailing list