[Mesa-dev] [PATCH mesa 3/5] bin: rewrite get_reviewers as opt-in only

[Dylan Baker]

Quoting Eric Engestrom (2018-06-01 09:50:34)
> On Thursday, 2018-05-31 10:04:25 -0700, Dylan Baker wrote:
> > Quoting Eric Engestrom (2018-05-31 07:04:33)
> > > Some people have mentioned they don't like the current get_reviewers.pl
> > > script (the one from the kernel) because it is way too greedy in its
> > > search for reviewers.
> > > 
> > > I tried to modify it to remove the git blame functionality, but I had
> > > a way too hard time figuring out what all this perl black magic does,
> > > and I figured I wouldn't be the only one, so I rewrote it in python
> > > instead, so that more people would be able to maintain it.
> > > 
> > > I kept only the basic functionality of parsing the REVIEWERS file, and
> > > only supporting the R: and F: tags for now (those were the only ones
> > > used in Mesa anyway). Patches can be passed in as arguments or via
> > > stdin, and `-f` can be used to simply find reviewers for the files
> > > given.
> > > 
> > > This should be a drop-in replacement for the old script, meant to be
> > > used with git; you can use it automatically by setting:
> > > $ git config sendemail.cccmd bin/get_reviewer.py
> > > 
> > > To reiterate, this script is now opt-in only, as it only returns
> > > reviewers added in REVIEWERS.
> > > 
> > > Signed-off-by: Eric Engestrom <eric.engestrom at intel.com>
> > > ---
> > > As a quirk that I decided to leave in, "F: /dev/null" can now be used to
> > > subscribe to files being added or deleted, which I'm thinking of adding
> > > to the build systems groups, so that build system reviewers can make
> > > sure their build system was updated accordingly. Thoughts?
> > > ---
> > >  bin/get_reviewer.py | 177 ++++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 177 insertions(+)
> > >  create mode 100755 bin/get_reviewer.py
> > > 
> > > diff --git a/bin/get_reviewer.py b/bin/get_reviewer.py
> > > new file mode 100755
> > > index 00000000000000000000..543086ba21505b81ea0c
> > > --- /dev/null
> > > +++ b/bin/get_reviewer.py
> > > @@ -0,0 +1,177 @@
> > > +#!/usr/bin/env python3
> > > +"""
> > > +Parser for REVIEWERS, designed for use with git.
> > > +
> > > +Use it automatically by configuring git like this:
> > > +  $ git config sendemail.cccmd bin/get_reviewer.py
> > > +
> > > +"""
> > > +
> > > +def parse_options(args):
> > > +    """
> > > +    Takes care of parsing the options and returning a simple dict with
> > > +    one member per option.
> > > +    """
> > > +    import argparse
> > 
> > Please don't put the imports in the function bodies, this looks like you're
> > doing dirty tricks to avoid circular imports.
> > 
> [snip]
> > > +if __name__ == '__main__':
> > > +    import sys
> > > +    main(sys.argv[1:], sys.stdin)
> > 
> > There shouldn't be any need to strip the first element of sys.argv for argparse,
> > it knows how to handle that 'get_reveiwers.py' is the first argument. There
> > shouldn't be a reason to pass it to argparse at all, since we passing all of the
> > arguments and argparse will just use sys.argv if it's passed no arguments.
> > 
> > There isn't really a reason to pass sys.stdin here either, since it's th eonly
> > option anyway.
> 
> My reasoning was the same for both of these: locality. I was trying to
> leave things as non-specific as possible. Importing things only when
> needed, passing arguments explicitly and stripping off irrelevant bits
> were all done so that the function that receives them doesn't have to
> care about where or how these things were handled, it just gets a list
> of strings and a file to read.
> 
> I'm happy to re-organise this to import everything globally and drop
> parameters like these if that's the best practice :)

I'd at least like have the imports at global scope. Usually when someone uses an
import inside a function in python it's to avoid a circular dependency, since
the import will be evaluated at runtime rather than at module initialization
time. I'm less concerned about the sys stuff, although it does seem a little
strange to add extra code to handle something that's already handled in the
code we're calling, but it's also not the end of the world :)

Dylan

> 
> I'll make a v2 next week with that and everything everyone will have
> mentioned then.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: signature
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180601/0e0137c0/attachment.sig>