* [edk2-devel] [edk2-stable202311][Patch 1/1] BaseTools/Scripts: Handle reviewer only case in GetMaintainer.py @ 2023-11-08 20:43 Michael D Kinney 2023-11-10 12:43 ` Leif Lindholm 0 siblings, 1 reply; 4+ messages in thread From: Michael D Kinney @ 2023-11-08 20:43 UTC (permalink / raw) To: devel; +Cc: Rebecca Cran, Liming Gao, Bob Feng, Yuwei Chen, Leif Lindholm REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4593 If a package only has reviewers and no maintainers, then also return the <default> maintainers. Update get_maintainers() to return maintainers, reviews, and lists separately instead of a single merged list to allow this module to be used by other scripts and distinguish types. Sort the list of output addresses alphabetically. Fix logic bug where maintainers was incorrectly added to lists. Cc: Rebecca Cran <rebecca@bsdio.com> Cc: Liming Gao <gaoliming@byosoft.com.cn> Cc: Bob Feng <bob.c.feng@intel.com> Cc: Yuwei Chen <yuwei.chen@intel.com> Cc: Leif Lindholm <quic_llindhol@quicinc.com> Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com> --- BaseTools/Scripts/GetMaintainer.py | 42 ++++++++++++++++++------------ 1 file changed, 26 insertions(+), 16 deletions(-) diff --git a/BaseTools/Scripts/GetMaintainer.py b/BaseTools/Scripts/GetMaintainer.py index d1e042c0afe4..b33546b10f21 100644 --- a/BaseTools/Scripts/GetMaintainer.py +++ b/BaseTools/Scripts/GetMaintainer.py @@ -76,6 +76,7 @@ def get_section_maintainers(path, section): """Returns a list with email addresses to any M: and R: entries matching the provided path in the provided section.""" maintainers = [] + reviewers = [] lists = [] nowarn_status = ['Supported', 'Maintained'] @@ -83,12 +84,18 @@ def get_section_maintainers(path, section): for status in section['status']: if status not in nowarn_status: print('WARNING: Maintained status for "%s" is \'%s\'!' % (path, status)) - for address in section['maintainer'], section['reviewer']: + for address in section['maintainer']: # Convert to list if necessary if isinstance(address, list): maintainers += address else: - lists += [address] + maintainers += [address] + for address in section['reviewer']: + # Convert to list if necessary + if isinstance(address, list): + reviewers += address + else: + reviewers += [address] for address in section['list']: # Convert to list if necessary if isinstance(address, list): @@ -96,32 +103,34 @@ def get_section_maintainers(path, section): else: lists += [address] - return maintainers, lists + return maintainers, reviewers, lists def get_maintainers(path, sections, level=0): """For 'path', iterates over all sections, returning maintainers for matching ones.""" maintainers = [] + reviewers = [] lists = [] for section in sections: - tmp_maint, tmp_lists = get_section_maintainers(path, section) - if tmp_maint: - maintainers += tmp_maint - if tmp_lists: - lists += tmp_lists + tmp_maint, tmp_review, tmp_lists = get_section_maintainers(path, section) + maintainers += tmp_maint + reviewers += tmp_review + lists += tmp_lists if not maintainers: # If no match found, look for match for (nonexistent) file # REPO.working_dir/<default> print('"%s": no maintainers found, looking for default' % path) if level == 0: - maintainers = get_maintainers('<default>', sections, level=level + 1) + maintainers, tmp_review, tmp_lists = get_maintainers('<default>', sections, level=level + 1) + reviewers += tmp_review + lists += tmp_lists else: print("No <default> maintainers set for project.") if not maintainers: return None - return maintainers + lists + return maintainers, reviewers, lists def parse_maintainers_line(line): """Parse one line of Maintainers.txt, returning any match group and its key.""" @@ -182,15 +191,16 @@ if __name__ == '__main__': else: FILES = get_modified_files(REPO, ARGS) - ADDRESSES = [] - + # Accumulate a sorted list of addresses + ADDRESSES = set([]) for file in FILES: print(file) - addresslist = get_maintainers(file, SECTIONS) - if addresslist: - ADDRESSES += addresslist + maintainers, reviewers, lists = get_maintainers(file, SECTIONS) + ADDRESSES |= set(maintainers + reviewers + lists) + ADDRESSES = list(ADDRESSES) + ADDRESSES.sort() - for address in list(OrderedDict.fromkeys(ADDRESSES)): + for address in ADDRESSES: if '<' in address and '>' in address: address = address.split('>', 1)[0] + '>' print(' %s' % address) -- 2.40.1.windows.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#110925): https://edk2.groups.io/g/devel/message/110925 Mute This Topic: https://groups.io/mt/102472591/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [edk2-devel] [edk2-stable202311][Patch 1/1] BaseTools/Scripts: Handle reviewer only case in GetMaintainer.py 2023-11-08 20:43 [edk2-devel] [edk2-stable202311][Patch 1/1] BaseTools/Scripts: Handle reviewer only case in GetMaintainer.py Michael D Kinney @ 2023-11-10 12:43 ` Leif Lindholm 2023-11-10 16:34 ` Michael D Kinney 0 siblings, 1 reply; 4+ messages in thread From: Leif Lindholm @ 2023-11-10 12:43 UTC (permalink / raw) To: Michael D Kinney; +Cc: devel, Rebecca Cran, Liming Gao, Bob Feng, Yuwei Chen On Wed, Nov 08, 2023 at 12:43:23 -0800, Michael D Kinney wrote: > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4593 > > If a package only has reviewers and no maintainers, then also > return the <default> maintainers. > > Update get_maintainers() to return maintainers, reviews, and > lists separately instead of a single merged list to allow this > module to be used by other scripts and distinguish types. > > Sort the list of output addresses alphabetically. > > Fix logic bug where maintainers was incorrectly added to lists. > > Cc: Rebecca Cran <rebecca@bsdio.com> > Cc: Liming Gao <gaoliming@byosoft.com.cn> > Cc: Bob Feng <bob.c.feng@intel.com> > Cc: Yuwei Chen <yuwei.chen@intel.com> > Cc: Leif Lindholm <quic_llindhol@quicinc.com> > Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com> > --- > BaseTools/Scripts/GetMaintainer.py | 42 ++++++++++++++++++------------ > 1 file changed, 26 insertions(+), 16 deletions(-) > > diff --git a/BaseTools/Scripts/GetMaintainer.py b/BaseTools/Scripts/GetMaintainer.py > index d1e042c0afe4..b33546b10f21 100644 > --- a/BaseTools/Scripts/GetMaintainer.py > +++ b/BaseTools/Scripts/GetMaintainer.py > @@ -76,6 +76,7 @@ def get_section_maintainers(path, section): > """Returns a list with email addresses to any M: and R: entries > matching the provided path in the provided section.""" > maintainers = [] > + reviewers = [] > lists = [] > nowarn_status = ['Supported', 'Maintained'] > > @@ -83,12 +84,18 @@ def get_section_maintainers(path, section): > for status in section['status']: > if status not in nowarn_status: > print('WARNING: Maintained status for "%s" is \'%s\'!' % (path, status)) > - for address in section['maintainer'], section['reviewer']: > + for address in section['maintainer']: > # Convert to list if necessary > if isinstance(address, list): > maintainers += address > else: > - lists += [address] > + maintainers += [address] That's a bugfix. Ought to be separate. (Cleverly hidden by concatentaing the results when we didn't care about keeping them separate other than for seeing if we'd found any humans.) > + for address in section['reviewer']: > + # Convert to list if necessary > + if isinstance(address, list): > + reviewers += address > + else: > + reviewers += [address] > for address in section['list']: > # Convert to list if necessary > if isinstance(address, list): > @@ -96,32 +103,34 @@ def get_section_maintainers(path, section): > else: > lists += [address] > > - return maintainers, lists > + return maintainers, reviewers, lists > > def get_maintainers(path, sections, level=0): > """For 'path', iterates over all sections, returning maintainers > for matching ones.""" > maintainers = [] > + reviewers = [] > lists = [] > for section in sections: > - tmp_maint, tmp_lists = get_section_maintainers(path, section) > - if tmp_maint: > - maintainers += tmp_maint > - if tmp_lists: > - lists += tmp_lists > + tmp_maint, tmp_review, tmp_lists = get_section_maintainers(path, section) > + maintainers += tmp_maint > + reviewers += tmp_review > + lists += tmp_lists Minor niggle at coding style cleanup as part of functional rework. > > if not maintainers: > # If no match found, look for match for (nonexistent) file > # REPO.working_dir/<default> > print('"%s": no maintainers found, looking for default' % path) > if level == 0: > - maintainers = get_maintainers('<default>', sections, level=level + 1) > + maintainers, tmp_review, tmp_lists = get_maintainers('<default>', sections, level=level + 1) > + reviewers += tmp_review > + lists += tmp_lists > else: > print("No <default> maintainers set for project.") > if not maintainers: > return None > > - return maintainers + lists Apart from the niggles mentioned above, I agree that this is a reasonable way of adding the required functionality without completely rewriting the existing code. (It does make me feel there must be a better way of writing it than I did, though.) > + return maintainers, reviewers, lists > > def parse_maintainers_line(line): > """Parse one line of Maintainers.txt, returning any match group and its key.""" > @@ -182,15 +191,16 @@ if __name__ == '__main__': > else: > FILES = get_modified_files(REPO, ARGS) > > - ADDRESSES = [] > - > + # Accumulate a sorted list of addresses > + ADDRESSES = set([]) > for file in FILES: > print(file) > - addresslist = get_maintainers(file, SECTIONS) > - if addresslist: > - ADDRESSES += addresslist > + maintainers, reviewers, lists = get_maintainers(file, SECTIONS) > + ADDRESSES |= set(maintainers + reviewers + lists) > + ADDRESSES = list(ADDRESSES) > + ADDRESSES.sort() > > - for address in list(OrderedDict.fromkeys(ADDRESSES)): > + for address in ADDRESSES: But the above doesn't seem to have any impact on the generated output at all. So I guess this is to enable the github work to utilise get_maintainers() directly while maintaining the separation of maintainer/reviewer/list? It feels to me like that change would be more clear as a separate commit from the one that breaks out reviewers from maintainers. I don't have a strong preference for the ordering. And it would probably also be less fragile (w.r.t. future edits) if the end result returned a dict instead of three lists. / Leif > if '<' in address and '>' in address: > address = address.split('>', 1)[0] + '>' > print(' %s' % address) > -- > 2.40.1.windows.1 > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#111036): https://edk2.groups.io/g/devel/message/111036 Mute This Topic: https://groups.io/mt/102472591/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [edk2-devel] [edk2-stable202311][Patch 1/1] BaseTools/Scripts: Handle reviewer only case in GetMaintainer.py 2023-11-10 12:43 ` Leif Lindholm @ 2023-11-10 16:34 ` Michael D Kinney 2023-11-10 16:35 ` Leif Lindholm 0 siblings, 1 reply; 4+ messages in thread From: Michael D Kinney @ 2023-11-10 16:34 UTC (permalink / raw) To: devel@edk2.groups.io, quic_llindhol@quicinc.com Cc: Rebecca Cran, Gao, Liming, Feng, Bob C, Chen, Christine, Kinney, Michael D Hi Leif, Agree with your points. I was trying to make minimal changes to address the reviewers with no maintainers case. Returning a dictionary would make more sense. A couple questions: 1) Do you want to see this patch broken up into a series, with the logic fix, reviewers with no maintainers feature, and code clean up in separate patches? 2) Is this change approved for edk2-stable202311? Thanks, Mike > -----Original Message----- > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Leif > Lindholm > Sent: Friday, November 10, 2023 4:44 AM > To: Kinney, Michael D <michael.d.kinney@intel.com> > Cc: devel@edk2.groups.io; Rebecca Cran <rebecca@bsdio.com>; Gao, > Liming <gaoliming@byosoft.com.cn>; Feng, Bob C <bob.c.feng@intel.com>; > Chen, Christine <yuwei.chen@intel.com> > Subject: Re: [edk2-devel] [edk2-stable202311][Patch 1/1] > BaseTools/Scripts: Handle reviewer only case in GetMaintainer.py > > On Wed, Nov 08, 2023 at 12:43:23 -0800, Michael D Kinney wrote: > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4593 > > > > If a package only has reviewers and no maintainers, then also > > return the <default> maintainers. > > > > Update get_maintainers() to return maintainers, reviews, and > > lists separately instead of a single merged list to allow this > > module to be used by other scripts and distinguish types. > > > > Sort the list of output addresses alphabetically. > > > > Fix logic bug where maintainers was incorrectly added to lists. > > > > Cc: Rebecca Cran <rebecca@bsdio.com> > > Cc: Liming Gao <gaoliming@byosoft.com.cn> > > Cc: Bob Feng <bob.c.feng@intel.com> > > Cc: Yuwei Chen <yuwei.chen@intel.com> > > Cc: Leif Lindholm <quic_llindhol@quicinc.com> > > Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com> > > --- > > BaseTools/Scripts/GetMaintainer.py | 42 ++++++++++++++++++--------- > --- > > 1 file changed, 26 insertions(+), 16 deletions(-) > > > > diff --git a/BaseTools/Scripts/GetMaintainer.py > b/BaseTools/Scripts/GetMaintainer.py > > index d1e042c0afe4..b33546b10f21 100644 > > --- a/BaseTools/Scripts/GetMaintainer.py > > +++ b/BaseTools/Scripts/GetMaintainer.py > > @@ -76,6 +76,7 @@ def get_section_maintainers(path, section): > > """Returns a list with email addresses to any M: and R: entries > > matching the provided path in the provided section.""" > > maintainers = [] > > + reviewers = [] > > lists = [] > > nowarn_status = ['Supported', 'Maintained'] > > > > @@ -83,12 +84,18 @@ def get_section_maintainers(path, section): > > for status in section['status']: > > if status not in nowarn_status: > > print('WARNING: Maintained status for "%s" is > \'%s\'!' % (path, status)) > > - for address in section['maintainer'], section['reviewer']: > > + for address in section['maintainer']: > > # Convert to list if necessary > > if isinstance(address, list): > > maintainers += address > > else: > > - lists += [address] > > + maintainers += [address] > > That's a bugfix. Ought to be separate. > (Cleverly hidden by concatentaing the results when we didn't care > about keeping them separate other than for seeing if we'd found any > humans.) > > > + for address in section['reviewer']: > > + # Convert to list if necessary > > + if isinstance(address, list): > > + reviewers += address > > + else: > > + reviewers += [address] > > for address in section['list']: > > # Convert to list if necessary > > if isinstance(address, list): > > @@ -96,32 +103,34 @@ def get_section_maintainers(path, section): > > else: > > lists += [address] > > > > - return maintainers, lists > > + return maintainers, reviewers, lists > > > > def get_maintainers(path, sections, level=0): > > """For 'path', iterates over all sections, returning > maintainers > > for matching ones.""" > > maintainers = [] > > + reviewers = [] > > lists = [] > > for section in sections: > > - tmp_maint, tmp_lists = get_section_maintainers(path, > section) > > - if tmp_maint: > > - maintainers += tmp_maint > > - if tmp_lists: > > - lists += tmp_lists > > + tmp_maint, tmp_review, tmp_lists = > get_section_maintainers(path, section) > > + maintainers += tmp_maint > > + reviewers += tmp_review > > + lists += tmp_lists > > Minor niggle at coding style cleanup as part of functional rework. > > > > > if not maintainers: > > # If no match found, look for match for (nonexistent) file > > # REPO.working_dir/<default> > > print('"%s": no maintainers found, looking for default' % > path) > > if level == 0: > > - maintainers = get_maintainers('<default>', sections, > level=level + 1) > > + maintainers, tmp_review, tmp_lists = > get_maintainers('<default>', sections, level=level + 1) > > + reviewers += tmp_review > > + lists += tmp_lists > > else: > > print("No <default> maintainers set for project.") > > if not maintainers: > > return None > > > > - return maintainers + lists > > Apart from the niggles mentioned above, I agree that this is a > reasonable way of adding the required functionality without completely > rewriting the existing code. (It does make me feel there must be a > better way of writing it than I did, though.) > > > + return maintainers, reviewers, lists > > > > def parse_maintainers_line(line): > > """Parse one line of Maintainers.txt, returning any match group > and its key.""" > > @@ -182,15 +191,16 @@ if __name__ == '__main__': > > else: > > FILES = get_modified_files(REPO, ARGS) > > > > - ADDRESSES = [] > > - > > + # Accumulate a sorted list of addresses > > + ADDRESSES = set([]) > > for file in FILES: > > print(file) > > - addresslist = get_maintainers(file, SECTIONS) > > - if addresslist: > > - ADDRESSES += addresslist > > + maintainers, reviewers, lists = get_maintainers(file, > SECTIONS) > > + ADDRESSES |= set(maintainers + reviewers + lists) > > + ADDRESSES = list(ADDRESSES) > > + ADDRESSES.sort() > > > > - for address in list(OrderedDict.fromkeys(ADDRESSES)): > > + for address in ADDRESSES: > > But the above doesn't seem to have any impact on the generated output > at all. So I guess this is to enable the github work to utilise > get_maintainers() directly while maintaining the separation of > maintainer/reviewer/list? > > It feels to me like that change would be more clear as a separate > commit from the one that breaks out reviewers from maintainers. > I don't have a strong preference for the ordering. > > And it would probably also be less fragile (w.r.t. future edits) if > the end result returned a dict instead of three lists. > > / > Leif > > > if '<' in address and '>' in address: > > address = address.split('>', 1)[0] + '>' > > print(' %s' % address) > > -- > > 2.40.1.windows.1 > > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#111040): https://edk2.groups.io/g/devel/message/111040 Mute This Topic: https://groups.io/mt/102472591/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [edk2-devel] [edk2-stable202311][Patch 1/1] BaseTools/Scripts: Handle reviewer only case in GetMaintainer.py 2023-11-10 16:34 ` Michael D Kinney @ 2023-11-10 16:35 ` Leif Lindholm 0 siblings, 0 replies; 4+ messages in thread From: Leif Lindholm @ 2023-11-10 16:35 UTC (permalink / raw) To: Kinney, Michael D, devel@edk2.groups.io Cc: Rebecca Cran, Gao, Liming, Feng, Bob C, Chen, Christine On 2023-11-10 16:34, Kinney, Michael D wrote: > Hi Leif, > > Agree with your points. I was trying to make minimal changes to address > the reviewers with no maintainers case. Returning a dictionary would make > more sense. > > A couple questions: > > 1) Do you want to see this patch broken up into a series, with the > logic fix, reviewers with no maintainers feature, and code clean > up in separate patches? Ideally, yes. It will be helpful if I need to try to understand these changes again in future :) > 2) Is this change approved for edk2-stable202311? Yes. Regards, Leif > Thanks, > > Mike > >> -----Original Message----- >> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Leif >> Lindholm >> Sent: Friday, November 10, 2023 4:44 AM >> To: Kinney, Michael D <michael.d.kinney@intel.com> >> Cc: devel@edk2.groups.io; Rebecca Cran <rebecca@bsdio.com>; Gao, >> Liming <gaoliming@byosoft.com.cn>; Feng, Bob C <bob.c.feng@intel.com>; >> Chen, Christine <yuwei.chen@intel.com> >> Subject: Re: [edk2-devel] [edk2-stable202311][Patch 1/1] >> BaseTools/Scripts: Handle reviewer only case in GetMaintainer.py >> >> On Wed, Nov 08, 2023 at 12:43:23 -0800, Michael D Kinney wrote: >>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4593 >>> >>> If a package only has reviewers and no maintainers, then also >>> return the <default> maintainers. >>> >>> Update get_maintainers() to return maintainers, reviews, and >>> lists separately instead of a single merged list to allow this >>> module to be used by other scripts and distinguish types. >>> >>> Sort the list of output addresses alphabetically. >>> >>> Fix logic bug where maintainers was incorrectly added to lists. >>> >>> Cc: Rebecca Cran <rebecca@bsdio.com> >>> Cc: Liming Gao <gaoliming@byosoft.com.cn> >>> Cc: Bob Feng <bob.c.feng@intel.com> >>> Cc: Yuwei Chen <yuwei.chen@intel.com> >>> Cc: Leif Lindholm <quic_llindhol@quicinc.com> >>> Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com> >>> --- >>> BaseTools/Scripts/GetMaintainer.py | 42 ++++++++++++++++++--------- >> --- >>> 1 file changed, 26 insertions(+), 16 deletions(-) >>> >>> diff --git a/BaseTools/Scripts/GetMaintainer.py >> b/BaseTools/Scripts/GetMaintainer.py >>> index d1e042c0afe4..b33546b10f21 100644 >>> --- a/BaseTools/Scripts/GetMaintainer.py >>> +++ b/BaseTools/Scripts/GetMaintainer.py >>> @@ -76,6 +76,7 @@ def get_section_maintainers(path, section): >>> """Returns a list with email addresses to any M: and R: entries >>> matching the provided path in the provided section.""" >>> maintainers = [] >>> + reviewers = [] >>> lists = [] >>> nowarn_status = ['Supported', 'Maintained'] >>> >>> @@ -83,12 +84,18 @@ def get_section_maintainers(path, section): >>> for status in section['status']: >>> if status not in nowarn_status: >>> print('WARNING: Maintained status for "%s" is >> \'%s\'!' % (path, status)) >>> - for address in section['maintainer'], section['reviewer']: >>> + for address in section['maintainer']: >>> # Convert to list if necessary >>> if isinstance(address, list): >>> maintainers += address >>> else: >>> - lists += [address] >>> + maintainers += [address] >> >> That's a bugfix. Ought to be separate. >> (Cleverly hidden by concatentaing the results when we didn't care >> about keeping them separate other than for seeing if we'd found any >> humans.) >> >>> + for address in section['reviewer']: >>> + # Convert to list if necessary >>> + if isinstance(address, list): >>> + reviewers += address >>> + else: >>> + reviewers += [address] >>> for address in section['list']: >>> # Convert to list if necessary >>> if isinstance(address, list): >>> @@ -96,32 +103,34 @@ def get_section_maintainers(path, section): >>> else: >>> lists += [address] >>> >>> - return maintainers, lists >>> + return maintainers, reviewers, lists >>> >>> def get_maintainers(path, sections, level=0): >>> """For 'path', iterates over all sections, returning >> maintainers >>> for matching ones.""" >>> maintainers = [] >>> + reviewers = [] >>> lists = [] >>> for section in sections: >>> - tmp_maint, tmp_lists = get_section_maintainers(path, >> section) >>> - if tmp_maint: >>> - maintainers += tmp_maint >>> - if tmp_lists: >>> - lists += tmp_lists >>> + tmp_maint, tmp_review, tmp_lists = >> get_section_maintainers(path, section) >>> + maintainers += tmp_maint >>> + reviewers += tmp_review >>> + lists += tmp_lists >> >> Minor niggle at coding style cleanup as part of functional rework. >> >>> >>> if not maintainers: >>> # If no match found, look for match for (nonexistent) file >>> # REPO.working_dir/<default> >>> print('"%s": no maintainers found, looking for default' % >> path) >>> if level == 0: >>> - maintainers = get_maintainers('<default>', sections, >> level=level + 1) >>> + maintainers, tmp_review, tmp_lists = >> get_maintainers('<default>', sections, level=level + 1) >>> + reviewers += tmp_review >>> + lists += tmp_lists >>> else: >>> print("No <default> maintainers set for project.") >>> if not maintainers: >>> return None >>> >>> - return maintainers + lists >> >> Apart from the niggles mentioned above, I agree that this is a >> reasonable way of adding the required functionality without completely >> rewriting the existing code. (It does make me feel there must be a >> better way of writing it than I did, though.) >> >>> + return maintainers, reviewers, lists >>> >>> def parse_maintainers_line(line): >>> """Parse one line of Maintainers.txt, returning any match group >> and its key.""" >>> @@ -182,15 +191,16 @@ if __name__ == '__main__': >>> else: >>> FILES = get_modified_files(REPO, ARGS) >>> >>> - ADDRESSES = [] >>> - >>> + # Accumulate a sorted list of addresses >>> + ADDRESSES = set([]) >>> for file in FILES: >>> print(file) >>> - addresslist = get_maintainers(file, SECTIONS) >>> - if addresslist: >>> - ADDRESSES += addresslist >>> + maintainers, reviewers, lists = get_maintainers(file, >> SECTIONS) >>> + ADDRESSES |= set(maintainers + reviewers + lists) >>> + ADDRESSES = list(ADDRESSES) >>> + ADDRESSES.sort() >>> >>> - for address in list(OrderedDict.fromkeys(ADDRESSES)): >>> + for address in ADDRESSES: >> >> But the above doesn't seem to have any impact on the generated output >> at all. So I guess this is to enable the github work to utilise >> get_maintainers() directly while maintaining the separation of >> maintainer/reviewer/list? >> >> It feels to me like that change would be more clear as a separate >> commit from the one that breaks out reviewers from maintainers. >> I don't have a strong preference for the ordering. >> >> And it would probably also be less fragile (w.r.t. future edits) if >> the end result returned a dict instead of three lists. >> >> / >> Leif >> >>> if '<' in address and '>' in address: >>> address = address.split('>', 1)[0] + '>' >>> print(' %s' % address) >>> -- >>> 2.40.1.windows.1 >>> >> >> >> >> > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#111041): https://edk2.groups.io/g/devel/message/111041 Mute This Topic: https://groups.io/mt/102472591/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-11-10 16:36 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-11-08 20:43 [edk2-devel] [edk2-stable202311][Patch 1/1] BaseTools/Scripts: Handle reviewer only case in GetMaintainer.py Michael D Kinney 2023-11-10 12:43 ` Leif Lindholm 2023-11-10 16:34 ` Michael D Kinney 2023-11-10 16:35 ` Leif Lindholm
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox