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

[Eric Engestrom]

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'll make a v2 next week with that and everything everyone will have
mentioned then.