[libcamera-devel] [PATCH v2 2/6] checkstyle: Exit with 1 status if issues are found

[Nicolas Dufresne]

Le vendredi 17 janvier 2020 à 23:58 +0200, Laurent Pinchart a écrit :
> Hi Nicolas,
> 
> Thank you for the patch.
> 
> On Fri, Jan 17, 2020 at 02:17:29PM -0500, Nicolas Dufresne wrote:
> > From: Nicolas Dufresne <nicolas.dufresne at collabora.com>
> > 
> > Makes the tool return 1 if there is any potential issues. This is
> > needed when using this tool for pre-commit hook in order to abort
> > the commit process.
> > 
> > Signed-off-by: Nicolas Dufresne <nicolas.dufresne at collabora.com>
> > ---
> >  utils/checkstyle.py | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> >  mode change 100755 => 100644 utils/checkstyle.py
> > 
> > diff --git a/utils/checkstyle.py b/utils/checkstyle.py
> > old mode 100755
> > new mode 100644
> > index 7edea25..e7375b3
> > --- a/utils/checkstyle.py
> > +++ b/utils/checkstyle.py
> > @@ -541,7 +541,7 @@ def check_style(top_level, commit):
> >      files = [f for f in files if len([p for p in patterns if
> > fnmatch.fnmatch(os.path.basename(f), p)])]
> >      if len(files) == 0:
> >          print("Commit doesn't touch source files, skipping")
> > -        return
> > +        return 0
> >  
> >      issues = 0
> >      for f in files:
> > @@ -554,6 +554,8 @@ def check_style(top_level, commit):
> >          print("%u potential style %s detected, please review" % \
> >                  (issues, 'issue' if issues == 1 else 'issues'))
> >  
> > +    return issues
> > +
> >  
> >  def extract_revlist(revs):
> >      """Extract a list of commits on which to operate from a revision or
> > revision
> > @@ -632,11 +634,12 @@ def main(argv):
> >  
> >      revlist = extract_revlist(args.revision_range)
> >  
> > +    issues = 0
> >      for commit in revlist:
> > -        check_style(top_level, commit)
> > +        issues += check_style(top_level, commit)
> >          print('')
> >  
> > -    return 0
> > +    return min(issues, 1)
> 
> I find this a bit difficult to read, but the python alternative to the C
> ternary operator would be
> 
> 	return issues and 1 or 0
> 
> which may not be more reable.
> 
> 	if issues:
> 		return 1
> 	else:
> 		return 0
> 
> may be an option, but not very nice either. I'll leave it up to you,
> maybe
> 
> 	# Return an error if any issues have been detected
> 	return min(issues, 0)

One of the two last suggestion will do, I have no preference, but not the first
one, that's just worst. Will you fix it why pushing ?

> 
> In any case,
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
> >  
> >  
> >  if __name__ == '__main__':