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 336EFA09E4; Thu, 22 Apr 2021 12:31:30 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id B0F6141D05; Thu, 22 Apr 2021 12:31:29 +0200 (CEST) Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by mails.dpdk.org (Postfix) with ESMTP id 09F87406A3 for ; Thu, 22 Apr 2021 12:31:27 +0200 (CEST) IronPort-SDR: CcfSeGm/yXksRSGhcOF8McGk1IepeBCl5/F7cD4F/I3ThBrHYSCssxHoAJdnUAppPHmO5npfLn UX/M4zZp2Y2g== X-IronPort-AV: E=McAfee;i="6200,9189,9961"; a="281192428" X-IronPort-AV: E=Sophos;i="5.82,242,1613462400"; d="scan'208";a="281192428" Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by fmsmga105.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 22 Apr 2021 03:31:26 -0700 IronPort-SDR: 5Tl9KDi2tTEf2usjysPcz/1vKPayjIoMvBUfw52ILGZO6Az4B1xL/EW17WaERsld402WuQmkAF NRjVRGbAk6Rg== X-IronPort-AV: E=Sophos;i="5.82,242,1613462400"; d="scan'208";a="524604385" Received: from bricha3-mobl.ger.corp.intel.com ([10.252.30.122]) by fmsmga001-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-SHA; 22 Apr 2021 03:31:24 -0700 Date: Thu, 22 Apr 2021 11:31:21 +0100 From: Bruce Richardson To: "Burakov, Anatoly" Cc: dev@dpdk.org, thomas@monjalon.net Message-ID: References: <20210421220358.2597249-1-thomas@monjalon.net> <20210422090211.320855-1-bruce.richardson@intel.com> <5af7d31b-332d-3dc9-a70b-54cbdb58c3ef@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <5af7d31b-332d-3dc9-a70b-54cbdb58c3ef@intel.com> Subject: Re: [dpdk-dev] [RFC PATCH] devtools: script to check meson indentation of lists 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 Sender: "dev" On Thu, Apr 22, 2021 at 11:21:26AM +0100, Burakov, Anatoly wrote: > On 22-Apr-21 10:58 AM, Bruce Richardson wrote: > > On Thu, Apr 22, 2021 at 10:40:37AM +0100, Burakov, Anatoly wrote: > > > On 22-Apr-21 10:02 AM, Bruce Richardson wrote: > > > > This is a draft script developed when I was working on the whitespace rework > > > > changes, since extended a little to attempt to fix some trailing comma issues. > > > > > > > > Signed-off-by: Bruce Richardson > > > > --- > > > > + if not line.endswith(',') and '#' not in line: > > > > + # TODO: support stripping comment and adding ',' > > > > + print(f'Error: Missing trailing "," in list at {filename}:{lineno + 1}') > > > > + contents[lineno] = line + ',\n' > > > > + line = contents[lineno].rstrip() > > > > > > What is the point of setting `line` here? > > > > > Because it allows us to make further checks using "line" as we add them > > later. > > The one question is whether it's worth using line as a shortcut for > > contents[lineno] or not. I'd tend to keep it, as it also allows us not to > > worry about the '\n' at the end. Then again, other rework might just change > > the whole script to strip off all the '\n' post-read and add them back > > again pre-write. > > > > Again, further cleanup work. > > Well, yes, i got that, it's just that as far as i can tell, the last "line = > ..." is at the end of the iteration, so there's no point in setting it. > Maybe i'm missing something :) No, you are not, and indeed it is at the very end. The fact it's there is to make sure it's not forgotten when adding in new checks. It also allows the checks to be reordered easier. Think of it like the trailing comma on a list! :-) > > > > > > + edit_count += 1 > > > > + return edit_count > > > > + > > > > + > > > > +def process_file(filename): > > > > + '''run checks on file "filename"''' > > > > + if VERBOSE: > > > > + print(f'Processing {filename}') > > > > + with open(filename) as f: > > > > + contents = f.readlines() > > > > > > I guess meson build files don't get too big so it's OK to read the entire > > > file in memory and then work on it, rather than go line by line... > > > > > This was a deliberate choice when starting the script. For now the script > > only checks indentation and formatting of lists, but ideally in future it > > should check other things, e.g. alphabetical order of lists, or formatting > > of other parts. Rather than checking it all in one go, the script is > > structured so that we can call multiple functions with "contents" each of > > which does the processing without constantly re-reading and rewriting the > > file. Something like sorting a list alphabetically also requires changing > > multiple lines at a time, which is easier to do with lists that when > > streaming input. > > Right, gotcha. > > > > > > > + > > > > + if check_indentation(filename, contents) > 0 and FIX: > > > > + print(f"Fixing {filename}") > > > > + with open(filename, 'w') as f: > > > > + f.writelines(contents) > > > > + > > > > + > > > > +def main(): > > > > + '''parse arguments and then call other functions to do work''' > > > > + global VERBOSE > > > > + global FIX > > > > > > Seems like globals are unnecessary here when you can just pass them into > > > process_file? > > > > > Yes, they can just be passed, but I actually prefer to have those as > > globals rather than having larger parameter lists. It's also possible that > > e.g. verbose, should need to be passed through multiple levels of functions. > > Personal preference, though really. > > Static analyzers (e.g. pylint) will complain about it, which is why i > usually avoid those unless really necessary :) For something like "verbose", > sure, one could argue that it's OK to have it as a global, but FIX is > definitely something that could be a parameter, as you don't seem to use it > anywhere other than in process_file(), nor would it seem likely that it will > be used in the future. > Yep, agreed. Fix can be just a parameter. It may be that it's kept as a global here is for consistency with verbose, but more likely it's just an oversight on my part. I will change it to parameter instead in any next revision. /Bruce