From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 0849643BA4; Fri, 23 Feb 2024 20:09:06 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id EA43741611; Fri, 23 Feb 2024 20:09:05 +0100 (CET) Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mails.dpdk.org (Postfix) with ESMTP id ECB21402ED for ; Fri, 23 Feb 2024 20:09:03 +0100 (CET) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 34941DA7; Fri, 23 Feb 2024 11:09:42 -0800 (PST) Received: from [192.168.50.86] (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 03A4E3F73F; Fri, 23 Feb 2024 11:09:02 -0800 (PST) Message-ID: Date: Fri, 23 Feb 2024 19:09:02 +0000 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird From: Luca Vizzarro Subject: Re: [PATCH 1/4] dts: constrain DPDK source flag To: =?UTF-8?Q?Juraj_Linke=C5=A1?= References: <20240122182611.1904974-1-luca.vizzarro@arm.com> <20240122182611.1904974-2-luca.vizzarro@arm.com> Content-Language: en-GB Cc: "dev@dpdk.org" In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org 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