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
