From: Luca Vizzarro <Luca.Vizzarro@arm.com>
To: "Juraj Linkeš" <juraj.linkes@pantheon.tech>
Cc: "dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [PATCH 1/4] dts: constrain DPDK source flag
Date: Fri, 23 Feb 2024 19:09:02 +0000 [thread overview]
Message-ID: <ed95e618-3688-4462-b043-ed2c71f49800@arm.com> (raw)
In-Reply-To: <CAOb5WZa9P_iaQJQd_qRyv=g1FtaYNw0dQ1cj2LHxxLqjEhBk7g@mail.gmail.com>
Hi Juraj,
Thank you for your review!
On 29/01/2024 11:47, Juraj Linkeš wrote:
> I didn't see the mutual exclusion being enforced in the code. From
> what I can tell, I could pass both --tarball FILEPATH and --revision
> and the former would be used without checking the latter.
Yep, it is enforced in the code, you may have missed it. The two
arguments are under the same mutual exclusion group in line 220:
dpdk_source = parser.add_mutually_exclusive_group(required=True)
When using both arguments `argparse` will automatically complain that
you can only use one or the other.
> whether `filepath` is valid
> Even though private methods don't get included in the API docs, I like
> to be consistent. In this case, it doesn't really detract (but maybe
> some disability would prove this wrong) while adding a bit (the fact
> that we're referencing the argument).
Yes, it is a good idea. Especially since this will work within IDEs.
> I think the name should either be _validate_tarball or
> _parse_tarball_path. The argument name is two words, so let's put an
> underscore between them.
Ack.
> I think this would read better as one sentence.
Ack.
> Since this is a patch with improvements, maybe we could add metavars
> to other arguments as well. It looks pretty good.
Sure, no problem!
> This removes the support for environment variables. It's possible we
> don't want the support for these two arguments. Maybe we don't need
> the support for variables at all. I'm leaning towards supporting the
> env variables, but we probably should refactor how they're done. The
> easiest would be to not do them with action, but just modifying the
> default value if set. That would be a worthwhile improvement.
I've tried to find a way to still keep them. But with arguments done
this way, it is somewhat hard to understand the provenance of the value,
whether it's set in the arguments, an environment variable or just the
default value. Therefore, give a useful error message to the user when
using something invalid. I'll try to come up with something as you
suggested, although I am not entirely convinced it'll be ideal.
> This would also probably read better as one sentence.
Ack.
> We shuffled the order of operations a bit and now the error message
> doesn't correspond.
Sorry, I don't think I am understanding what you are referring to
exactly. What do you mean?
Best,
Luca
next prev parent reply other threads:[~2024-02-23 19:09 UTC|newest]
Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-01-22 18:26 [PATCH 0/4] dts: error and usage improvements Luca Vizzarro
2024-01-22 18:26 ` [PATCH 1/4] dts: constrain DPDK source flag Luca Vizzarro
2024-01-29 11:47 ` Juraj Linkeš
2024-02-23 19:09 ` Luca Vizzarro [this message]
2024-03-01 10:22 ` Juraj Linkeš
2024-01-22 18:26 ` [PATCH 2/4] dts: customise argparse error message Luca Vizzarro
2024-01-29 13:04 ` Juraj Linkeš
2024-02-23 19:12 ` Luca Vizzarro
2024-02-26 9:09 ` Juraj Linkeš
2024-01-22 18:26 ` [PATCH 3/4] dts: show help when DTS is ran without args Luca Vizzarro
2024-01-22 18:26 ` [PATCH 4/4] dts: log stderr with failed remote commands Luca Vizzarro
2024-01-29 13:10 ` Juraj Linkeš
2024-02-23 19:19 ` Luca Vizzarro
2024-02-26 9:05 ` Juraj Linkeš
2024-03-18 17:17 ` [PATCH v2 0/3] dts: error and usage improvements Luca Vizzarro
2024-03-18 17:17 ` [PATCH v2 1/3] dts: rework arguments framework Luca Vizzarro
2024-04-04 9:25 ` Juraj Linkeš
2024-04-09 15:14 ` Luca Vizzarro
2024-04-10 9:46 ` Juraj Linkeš
2024-03-18 17:17 ` [PATCH v2 2/3] dts: constrain DPDK source argument Luca Vizzarro
2024-03-18 17:17 ` [PATCH v2 3/3] dts: store stderr in RemoteCommandExecutionError Luca Vizzarro
2024-05-14 11:44 ` [PATCH v3 0/3] error and usage improvements Luca Vizzarro
2024-05-14 11:44 ` [PATCH v3 1/3] dts: rework arguments framework Luca Vizzarro
2024-05-14 11:55 ` [PATCH v3] " Luca Vizzarro
2024-05-14 11:44 ` [PATCH v3 2/3] dts: constrain DPDK source argument Luca Vizzarro
2024-05-14 11:44 ` [PATCH v3 3/3] dts: store stderr in RemoteCommandExecutionError Luca Vizzarro
2024-05-14 12:04 ` [PATCH v4 0/3] error and usage improvements Luca Vizzarro
2024-05-14 12:05 ` [PATCH v4 1/3] dts: update mypy static checker Luca Vizzarro
2024-05-14 12:05 ` [PATCH v4 2/3] dts: clean up config types Luca Vizzarro
2024-05-14 12:05 ` [PATCH v4 3/3] dts: rework arguments framework Luca Vizzarro
2024-05-14 12:10 ` [PATCH v5 0/3] error and usage improvements Luca Vizzarro
2024-05-14 12:10 ` [PATCH v5 1/3] dts: rework arguments framework Luca Vizzarro
2024-05-14 12:10 ` [PATCH v5 2/3] dts: constrain DPDK source argument Luca Vizzarro
2024-05-14 12:10 ` [PATCH v5 3/3] dts: store stderr in RemoteCommandExecutionError Luca Vizzarro
2024-05-14 15:26 ` [PATCH v5 0/3] error and usage improvements Luca Vizzarro
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=ed95e618-3688-4462-b043-ed2c71f49800@arm.com \
--to=luca.vizzarro@arm.com \
--cc=dev@dpdk.org \
--cc=juraj.linkes@pantheon.tech \
/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).