[Patch, v2] contrib/mklog.py: Improve PR handling (was: Re: git gcc-commit-mklog doesn't extract PR number to ChangeLog)

[Martin Liška]

On 6/21/21 10:37 AM, Tobias Burnus wrote:
> On 21.06.21 10:09, Martin Liška wrote:
> 
>> $ pytest test_mklog.py
>> FAILED test_mklog.py::TestMklog::test_sorting - AssertionError: assert
>> '\n\tPR 50209...New test.\n\n' == 'gcc/ChangeLo...New test.\n\n'
> Aha, missed that there is indeed a testsuite - nice!
>> $ flake8 mklog.py
>> mklog.py:187:23: Q000 Remove bad quotes
> I have now filled:
> https://bugs.launchpad.net/ubuntu/+source/python-pytest-flake8/+bug/1933075
> 
>>> +            # PR number in the file name
>>> +            fname = os.path.basename(file.path)
>>
>> This is a dead code.
>>
>>> + fname = os.path.splitext(fname)[0]
>>> +            m = pr_filename_regex.search(fname)
> It does not look like dead code to me.

Hello.

The code is weird as os.path.basename returns:

In [5]: os.path.basename('/tmp/a/b/c.txt')
Out[5]: 'c.txt'

why do you need os.path.splitext(fname) call?


>>> + parser.add_argument('-b', '--pr-numbers', action='append',
>>> +                        help='Add the specified PRs (comma separated)')
>>
>> Do we really want to support '-b 1 -b 2' and also -b '1,2' formats?
>> Seems to me quite
>> complicated.
> 
> I don't have a strong opinion. I started with '-b 123,245', believing
> that the syntax is fine. But then I realized that without '-p'
> specifying multiple '-b' looks better by having multiple '-b' if 'PR
> <component>/'  (needed for -p as the string is than taken as is). Thus,
> I ended up supporting either variant.

I would start with -b 1,2,3,4 syntax. It will be likely easier for git alias integration.

Martin

> 
> But I also happily drop the ',' support.
> 
> Change: One quote change, one test_mklog update.
> 
> Tobias
> 
> -----------------
> Mentor Graphics (Deutschland) GmbH, Arnulfstrasse 201, 80634 München Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Frank Thürauf