Re: [PATCH 1/4] dts: constrain DPDK source flag

[Luca Vizzarro <Luca.Vizzarro@arm.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