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.engest...@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.
signature.asc
Description: signature
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev