From: "Juraj Linkeš" <juraj.linkes@pantheon.tech>
To: Adam Hassick <ahassick@iol.unh.edu>
Cc: ci@dpdk.org
Subject: Re: [PATCH v2 1/2] tools: Add script to create artifacts
Date: Mon, 15 Jan 2024 14:07:05 +0100 [thread overview]
Message-ID: <CAOb5WZbFAB5_nZRvUtz7G7W5poLDjfGz0AYDh2pkD=ryo+VN3A@mail.gmail.com> (raw)
In-Reply-To: <20240112224014.30955-2-ahassick@iol.unh.edu>
> + 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.
next prev parent reply other threads:[~2024-01-15 13:07 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-01-12 22:40 [PATCH v2 0/2] Add a script to create series artifacts Adam Hassick
2024-01-12 22:40 ` [PATCH v2 1/2] tools: Add script to create artifacts Adam Hassick
2024-01-15 13:07 ` Juraj Linkeš [this message]
2024-01-12 22:40 ` [PATCH v2 2/2] config: Add example config file Adam Hassick
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAOb5WZbFAB5_nZRvUtz7G7W5poLDjfGz0AYDh2pkD=ryo+VN3A@mail.gmail.com' \
--to=juraj.linkes@pantheon.tech \
--cc=ahassick@iol.unh.edu \
--cc=ci@dpdk.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).