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 (#111036): https://edk2.groups.io/g/devel/message/111036 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] -=-=-=-=-=-=-=-=-=-=-=-