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.kin...@intel.com> > Cc: devel@edk2.groups.io; Rebecca Cran <rebe...@bsdio.com>; Gao, > Liming <gaolim...@byosoft.com.cn>; Feng, Bob C <bob.c.f...@intel.com>; > Chen, Christine <yuwei.c...@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 <rebe...@bsdio.com> > > Cc: Liming Gao <gaolim...@byosoft.com.cn> > > Cc: Bob Feng <bob.c.f...@intel.com> > > Cc: Yuwei Chen <yuwei.c...@intel.com> > > Cc: Leif Lindholm <quic_llind...@quicinc.com> > > Signed-off-by: Michael D Kinney <michael.d.kin...@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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
