Re: [dpdk-dev] [PATCH v2 1/2] devtools: script to check meson indentation of lists

["Burakov, Anatoly" <anatoly.burakov@intel.com>]

On 26-Apr-21 3:05 PM, Bruce Richardson wrote:
> On Mon, Apr 26, 2021 at 02:40:25PM +0100, Burakov, Anatoly wrote:
>> On 26-Apr-21 11:54 AM, Bruce Richardson wrote:
>>> This is a script to fix up minor formatting issues in meson files.
>>> It scans for, and can optionally fix, indentation issues and missing
>>> trailing commas in the lists in meson.build files. It also detects,
>>> and can fix, multi-line lists where more than one entry appears on a
>>> line.
>>>
>>> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
>>> ---
>>
>> <snip>
>>
>>> +def split_code_comments(line):
>>> +    'splits a line into a code part and a comment part, returns (code, comment) tuple'
>>> +    if line.lstrip().startswith('#'):
>>> +        return ('', line)
>>> +    elif '#' in line and '#include' not in line:  # catch 99% of cases, not 100% > +        idx = line.index('#')
>>> +        while (line[idx - 1].isspace()):
>>> +            idx -= 1
>>> +        return line[:idx], line[idx:]
>>
>>
>> I think this could be simplified with regex:
>>
>> # find any occurrences of '#' but only if it's not an '#include'
>> if not re.search(r'#(?!include)', line)
>>      return line, ''
>> return line.split('#', maxsplit=1)
> 
> Not sure that is simpler, and just splitting on '#' is actually not what we
> want either.
> 
> Firstly, while r'#(?!include)' is not a massively complex regex, just
> checking for "'#' in line and '#include' not in line" is just easier to
> read for most mortals.
> 
> In terms of the split, I did initially do as you have here and split on
> '#', but we don't actually want that, because we want to preserve the
> whitespace in the line before the comment too - as part of the comment, not
> the code. This is why after finding the '#' we walk backwards to find the
> end of the code and find that as the split point. It then saves us worrying
> about any strips() breaking any comment alignment the user has explicitly
> set up. Not using split also means that we can just merge the strings back
> with '+' rather than having to use "'#'.join()".

Fair enough so!

> 
>>
>>> +    else:
>>> +        return (line, '')
>>> +
>>> +
>>> +def setline(contents, index, value):
>>> +    'sets the contents[index] to value. Returns the line, along with code and comments part'
>>> +    line = contents[index] = value
>>> +    code, comments = split_code_comments(line)
>>> +    return line, code, comments
>>> +
>>> +
>>> +def check_indentation(filename, contents):
>>> +    '''check that a list or files() is correctly indented'''
>>> +    infiles = False
>>> +    inlist = False
>>> +    edit_count = 0
>>> +    for lineno, line in enumerate(contents):
>>> +        code, comments = split_code_comments(line)
>>
>> Nitpicking, but maybe instead of calling strip() all over the place, just
>> count the number of spaces and strip right at the outset? E.g. something
>> like:
>>
>> stripped = code.strip()
>> line_indent = len(code) - len(stripped)
>>
>> You can then reason about indent levels by comparing stripped to code
>> afterwards, and avoid doing this:
>>
>>> +            # skip further subarrays or lists
>>> +            if '[' in code or ']' in code:
>>> +                continue
>>> +            if not code.startswith(indent) or code[len(indent)] == ' ':
>>
>> Opting to just check the indent size you calculated initially. Unless i'm
>> missing something :)
>>
>> You could also increment edit_count if `calculated indent + stripped` is
>> equal to `code`. Seems easier logic than raw string manipulation you're
>> going for here...
>>
>> <snip>
> 
> Interesting. That could be a good approach alright. If I do a V3 (not
> guaranteed for this release) I can try taking that idea on board.
> 
>>
>>> +def process_file(filename, fix):
>>> +    '''run checks on file "filename"'''
>>> +    if VERBOSE:
>>> +        print(f'Processing {filename}')
>>> +    with open(filename) as f:
>>> +        contents = [ln.rstrip() for ln in f.readlines()]
>>
>> So any trailing whitespace gets automatically and silently fixed?
>>
> Hadn't actually thought of that, but yes, that will happen if --fix is
> given and other changes are made to the file. Ideally, that should be fixed
> to "non-silently" do so, but I'd view it as low priority since other tools
> tend to be good at flagging trailing whitespace issues anyway.

Yeah, i think there's not a lot of trailing whitespace there in the 
first place, so probably a non issue.

> 
>>> +
>>> +    if check_indentation(filename, contents) > 0 and fix:
>>> +        print(f"Fixing {filename}")
>>> +        with open(filename, 'w') as f:
>>> +            f.writelines([f'{ln}\n' for ln in contents])
>>
>> Something seems suspect here. So, if `fix` is *not* specified, the script
>> just opens the file, reads it, and... does nothing else?
>>
> No, it prints out all the errors without actually fixing them.
> 

Oh, right, check_indentation will run first. Never mind!

In any case,

Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>

-- 
Thanks,
Anatoly