[libcamera-devel,v2,2/6] checkstyle: Exit with 1 status if issues are found
diff mbox series

Message ID 20200117191733.198897-3-nicolas@ndufresne.ca
State Superseded
Headers show
Series
  • Add the ability to do pre-commit style check
Related show

Commit Message

Nicolas Dufresne Jan. 17, 2020, 7:17 p.m. UTC 
From: Nicolas Dufresne <nicolas.dufresne@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@collabora.com>
---
 utils/checkstyle.py | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)
 mode change 100755 => 100644 utils/checkstyle.py

Comments

Niklas Söderlund Jan. 17, 2020, 9:30 p.m. UTC | #1 
Hi Nicolas,

Thanks for your work.

On 2020-01-17 14:17:29 -0500, Nicolas Dufresne wrote:
> From: Nicolas Dufresne <nicolas.dufresne@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@collabora.com>

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

> ---
>  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)
>  
>  
>  if __name__ == '__main__':
> -- 
> 2.24.1
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Laurent Pinchart Jan. 17, 2020, 9:58 p.m. UTC | #2 
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@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@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)

In any case,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  
>  
>  if __name__ == '__main__':
Nicolas Dufresne Jan. 17, 2020, 10:07 p.m. UTC | #3 
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@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@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@ideasonboard.com>
> 
> >  
> >  
> >  if __name__ == '__main__':
Laurent Pinchart Jan. 17, 2020, 10:14 p.m. UTC | #4 
Hi Nicolas,

On Fri, Jan 17, 2020 at 05:07:40PM -0500, Nicolas Dufresne wrote:
> Le vendredi 17 janvier 2020 à 23:58 +0200, Laurent Pinchart a écrit :
> > On Fri, Jan 17, 2020 at 02:17:29PM -0500, Nicolas Dufresne wrote:
> > > From: Nicolas Dufresne <nicolas.dufresne@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@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 ?

If you don't see a need to send a v3 due to other patches in this
series, I'll fix this (and possibly other small issues in other patches)
when pushing, yes.

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

Patch
diff mbox series 

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)
 
 
 if __name__ == '__main__':