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 49599438CF; Mon, 15 Jan 2024 14:07:18 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 432E3402C0; Mon, 15 Jan 2024 14:07:18 +0100 (CET) Received: from mail-ej1-f45.google.com (mail-ej1-f45.google.com [209.85.218.45]) by mails.dpdk.org (Postfix) with ESMTP id 48471402A2 for ; Mon, 15 Jan 2024 14:07:16 +0100 (CET) Received: by mail-ej1-f45.google.com with SMTP id a640c23a62f3a-a2d7e2e7fe0so223827766b.1 for ; Mon, 15 Jan 2024 05:07:16 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pantheon.tech; s=google; t=1705324036; x=1705928836; darn=dpdk.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=bwo+ILwq83r3/V/K0EF6U9NRROAAIgeDuX/aG7emAjM=; b=K1cWCQOJtBvqaR3pljaC7dOuHlJwoN/RR55WusPUAT/1b3zCmEYI6Pxl9N2aRFMFvm hpUcs/An1lI3ka/hgFwLsRTE9TV2Rb/FUAzaQpEFBKO1EFQYHRq5u5CRXXiNbcGUGbNp s3oUlvdcjrszE1LlEnncGQOIfffpzAjswb4RTPXs/lys4G6FFsoTkTmwCIMVzoN8YPv+ yB7yIneBxBzQVY32aE4MbAreqt4kq6TtKqPX85UWi7phrYTezg1V5R762pv0YJLVn9J6 r1YWuVKGGYIpWlpABIEWzU1MLsuaCOZ9MlI69GUh7uGLY1ylp9VklsZqOrt0M27iJX17 gPlg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1705324036; x=1705928836; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=bwo+ILwq83r3/V/K0EF6U9NRROAAIgeDuX/aG7emAjM=; b=foyCkWJDbrSL1mIh13DCtglkGS4uSrrSeIKr7fkwtcGzfk5xA8eYR9+6ASi1px1fwS ebnO0FzkP6MpPFhPo+sSY9pKrQ59bGJ90CZMugeFpPdUZbEAQ89fhMlBcR4l9IR82HoP heP8Aah420duHxbD197lOhWLsZmq1309+eAiLFfAbUhbQ6Iz0m0ZFQomUl0reFWDnDY7 MmvL1DhtMLvsqDbQkDx8H40c/kj3Ow1Uiu986jyBgVzs4asEGpb3GHpaRFSO4s0mfhTS 02E4iUpsp1+0QVpaIFcuVYLKYTBvwZZJRh8p+ouG5a2zMXvLogAMvSyU1OI6PrWXYnds gjdA== X-Gm-Message-State: AOJu0YwM88rScyZ0pcB1m4uyqf3KEkm+IJ1fZ5THhySvVV+YJGYy9Lsx 5vKWaY2waOaMN2/ifTUqOndzaqba8pFitGAQxS4/l5KWbuM1AYkM106XnChM+7LLTQ== X-Google-Smtp-Source: AGHT+IFXgSSWJkk4mABFs2Do+RRl7fBVSkS/Iftqd1nPAc0NYoyB9s5A7uEcdgkr7PIbX3Cl+A2qJU/hABOz/QmNfwI= X-Received: by 2002:a17:907:b002:b0:a28:b985:8da0 with SMTP id fu2-20020a170907b00200b00a28b9858da0mr3950827ejc.22.1705324036042; Mon, 15 Jan 2024 05:07:16 -0800 (PST) MIME-Version: 1.0 References: <20240112224014.30955-1-ahassick@iol.unh.edu> <20240112224014.30955-2-ahassick@iol.unh.edu> In-Reply-To: <20240112224014.30955-2-ahassick@iol.unh.edu> From: =?UTF-8?Q?Juraj_Linke=C5=A1?= Date: Mon, 15 Jan 2024 14:07:05 +0100 Message-ID: Subject: Re: [PATCH v2 1/2] tools: Add script to create artifacts To: Adam Hassick Cc: ci@dpdk.org Content-Type: text/plain; charset="UTF-8" X-BeenThere: ci@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK CI discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: ci-bounces@dpdk.org > + 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. > +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). > +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.