Re: [dpdk-dev] [RFC PATCH] devtools: script to check meson indentation of lists

[Bruce Richardson <bruce.richardson@intel.com>]

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 <bruce.richardson@intel.com>
> > > > ---

<snip>

> > > > +            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