[libcamera-devel,v2,3/6] checkstyle: Introduce a Commit class
diff mbox series

Message ID 20200117191733.198897-4-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>

This introduce a Commit class used in the final revlist list. All the
git command are moved into that class. This class will be used to
introduce new type of commit (index and amendment) needed to implement
pre-commit hook support.

Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
---
 utils/checkstyle.py | 43 +++++++++++++++++++++++++++++--------------
 1 file changed, 29 insertions(+), 14 deletions(-)

Comments

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

Thanks for your work.

On 2020-01-17 14:17:30 -0500, Nicolas Dufresne wrote:
> From: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> 
> This introduce a Commit class used in the final revlist list. All the
> git command are moved into that class. This class will be used to
> introduce new type of commit (index and amendment) needed to implement
> pre-commit hook support.
> 
> Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>

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

> ---
>  utils/checkstyle.py | 43 +++++++++++++++++++++++++++++--------------
>  1 file changed, 29 insertions(+), 14 deletions(-)
> 
> diff --git a/utils/checkstyle.py b/utils/checkstyle.py
> index e7375b3..fb865c8 100644
> --- a/utils/checkstyle.py
> +++ b/utils/checkstyle.py
> @@ -458,12 +458,34 @@ class StripTrailingSpaceFormatter(Formatter):
>  # Style checking
>  #
>  
> +class Commit:
> +    commit = None
> +
> +    def __init__(self, commit):
> +        self.commit = commit
> +
> +    def get_info(self, top_level):
> +        # Get the commit title and list of files.
> +        ret = subprocess.run(['git', 'show', '--pretty=oneline', '--name-only',
> +                              self.commit],
> +                             stdout=subprocess.PIPE).stdout.decode('utf-8')
> +        files = ret.splitlines()
> +        return files[0], files[1:]
> +
> +    def get_diff(self, top_level, filename):
> +        return subprocess.run(['git', 'diff', '%s~..%s' % (self.commit, self.commit),
> +                               '--', '%s/%s' % (top_level, filename)],
> +                              stdout=subprocess.PIPE).stdout.decode('utf-8')
> +
> +    def get_file(self, filename):
> +        return subprocess.run(['git', 'show', '%s:%s' % (self.commit, filename)],
> +                              stdout=subprocess.PIPE).stdout.decode('utf-8')
> +
> +
>  def check_file(top_level, commit, filename):
>      # Extract the line numbers touched by the commit.
> -    diff = subprocess.run(['git', 'diff', '%s~..%s' % (commit, commit), '--',
> -                           '%s/%s' % (top_level, filename)],
> -                          stdout=subprocess.PIPE).stdout
> -    diff = diff.decode('utf-8').splitlines(True)
> +    diff = commit.get_diff(top_level, filename)
> +    diff = diff.splitlines(True)
>      commit_diff = parse_diff(diff)
>  
>      lines = []
> @@ -476,9 +498,7 @@ def check_file(top_level, commit, filename):
>  
>      # Format the file after the commit with all formatters and compute the diff
>      # between the unformatted and formatted contents.
> -    after = subprocess.run(['git', 'show', '%s:%s' % (commit, filename)],
> -                           stdout=subprocess.PIPE).stdout
> -    after = after.decode('utf-8')
> +    after = commit.get_file(filename)
>  
>      formatted = after
>      for formatter in Formatter.formatters(filename):
> @@ -522,12 +542,7 @@ def check_file(top_level, commit, filename):
>  
>  
>  def check_style(top_level, commit):
> -    # Get the commit title and list of files.
> -    ret = subprocess.run(['git', 'show', '--pretty=oneline','--name-only', commit],
> -                         stdout=subprocess.PIPE)
> -    files = ret.stdout.decode('utf-8').splitlines()
> -    title = files[0]
> -    files = files[1:]
> +    title, files = commit.get_info(top_level)
>  
>      separator = '-' * len(title)
>      print(separator)
> @@ -576,7 +591,7 @@ def extract_revlist(revs):
>          revlist = ret.stdout.decode('utf-8').splitlines()
>          revlist.reverse()
>  
> -    return revlist
> +    return [Commit(x) for x in revlist]
>  
>  
>  def git_top_level():
> -- 
> 2.24.1
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Laurent Pinchart Jan. 17, 2020, 10:12 p.m. UTC | #2 
Hi Nicolas,

Thank you for the patch.

On Fri, Jan 17, 2020 at 02:17:30PM -0500, Nicolas Dufresne wrote:
> From: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> 
> This introduce a Commit class used in the final revlist list. All the
> git command are moved into that class. This class will be used to
> introduce new type of commit (index and amendment) needed to implement
> pre-commit hook support.
> 
> Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> ---
>  utils/checkstyle.py | 43 +++++++++++++++++++++++++++++--------------
>  1 file changed, 29 insertions(+), 14 deletions(-)
> 
> diff --git a/utils/checkstyle.py b/utils/checkstyle.py
> index e7375b3..fb865c8 100644
> --- a/utils/checkstyle.py
> +++ b/utils/checkstyle.py
> @@ -458,12 +458,34 @@ class StripTrailingSpaceFormatter(Formatter):
>  # Style checking
>  #
>  
> +class Commit:
> +    commit = None

This isn't needed. This field would be accessed through Commit.commit,
which isn't equivalent to self.commit below.

> +
> +    def __init__(self, commit):
> +        self.commit = commit
> +
> +    def get_info(self, top_level):
> +        # Get the commit title and list of files.
> +        ret = subprocess.run(['git', 'show', '--pretty=oneline', '--name-only',
> +                              self.commit],
> +                             stdout=subprocess.PIPE).stdout.decode('utf-8')
> +        files = ret.splitlines()
> +        return files[0], files[1:]

How about

	ret = ret.splitlines()
	title = ret[0]
	files = ret[1:]
	return title, files

to show what we're returning ?

> +
> +    def get_diff(self, top_level, filename):
> +        return subprocess.run(['git', 'diff', '%s~..%s' % (self.commit, self.commit),
> +                               '--', '%s/%s' % (top_level, filename)],
> +                              stdout=subprocess.PIPE).stdout.decode('utf-8')
> +
> +    def get_file(self, filename):
> +        return subprocess.run(['git', 'show', '%s:%s' % (self.commit, filename)],
> +                              stdout=subprocess.PIPE).stdout.decode('utf-8')
> +
> +
>  def check_file(top_level, commit, filename):
>      # Extract the line numbers touched by the commit.
> -    diff = subprocess.run(['git', 'diff', '%s~..%s' % (commit, commit), '--',
> -                           '%s/%s' % (top_level, filename)],
> -                          stdout=subprocess.PIPE).stdout
> -    diff = diff.decode('utf-8').splitlines(True)
> +    diff = commit.get_diff(top_level, filename)
> +    diff = diff.splitlines(True)
>      commit_diff = parse_diff(diff)
>  
>      lines = []
> @@ -476,9 +498,7 @@ def check_file(top_level, commit, filename):
>  
>      # Format the file after the commit with all formatters and compute the diff
>      # between the unformatted and formatted contents.
> -    after = subprocess.run(['git', 'show', '%s:%s' % (commit, filename)],
> -                           stdout=subprocess.PIPE).stdout
> -    after = after.decode('utf-8')
> +    after = commit.get_file(filename)
>  
>      formatted = after
>      for formatter in Formatter.formatters(filename):
> @@ -522,12 +542,7 @@ def check_file(top_level, commit, filename):
>  
>  
>  def check_style(top_level, commit):
> -    # Get the commit title and list of files.
> -    ret = subprocess.run(['git', 'show', '--pretty=oneline','--name-only', commit],
> -                         stdout=subprocess.PIPE)
> -    files = ret.stdout.decode('utf-8').splitlines()
> -    title = files[0]
> -    files = files[1:]
> +    title, files = commit.get_info(top_level)
>  
>      separator = '-' * len(title)
>      print(separator)
> @@ -576,7 +591,7 @@ def extract_revlist(revs):
>          revlist = ret.stdout.decode('utf-8').splitlines()
>          revlist.reverse()
>  
> -    return revlist
> +    return [Commit(x) for x in revlist]

As you're returning commits, should this function be renamed to
extract_commits ?

With these issues addressed,

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

>  
>  
>  def git_top_level():
Nicolas Dufresne Jan. 17, 2020, 10:32 p.m. UTC | #3 
Le samedi 18 janvier 2020 à 00:12 +0200, Laurent Pinchart a écrit :
> Hi Nicolas,
> 
> Thank you for the patch.
> 
> On Fri, Jan 17, 2020 at 02:17:30PM -0500, Nicolas Dufresne wrote:
> > From: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > 
> > This introduce a Commit class used in the final revlist list. All the
> > git command are moved into that class. This class will be used to
> > introduce new type of commit (index and amendment) needed to implement
> > pre-commit hook support.
> > 
> > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > ---
> >  utils/checkstyle.py | 43 +++++++++++++++++++++++++++++--------------
> >  1 file changed, 29 insertions(+), 14 deletions(-)
> > 
> > diff --git a/utils/checkstyle.py b/utils/checkstyle.py
> > index e7375b3..fb865c8 100644
> > --- a/utils/checkstyle.py
> > +++ b/utils/checkstyle.py
> > @@ -458,12 +458,34 @@ class StripTrailingSpaceFormatter(Formatter):
> >  # Style checking
> >  #
> >  
> > +class Commit:
> > +    commit = None
> 
> This isn't needed. This field would be accessed through Commit.commit,
> which isn't equivalent to self.commit below.

Oops, you are right. I'm rusty in python.

> 
> > +
> > +    def __init__(self, commit):
> > +        self.commit = commit
> > +
> > +    def get_info(self, top_level):
> > +        # Get the commit title and list of files.
> > +        ret = subprocess.run(['git', 'show', '--pretty=oneline', '--name-
> > only',
> > +                              self.commit],
> > +                             stdout=subprocess.PIPE).stdout.decode('utf-8')
> > +        files = ret.splitlines()
> > +        return files[0], files[1:]
> 
> How about
> 
> 	ret = ret.splitlines()
> 	title = ret[0]
> 	files = ret[1:]
> 	return title, files
> 
> to show what we're returning ?

I'd use a comment for that instead (it's interpreted language after all), what
about:

  # returning title and files list as a tuple

> 
> > +
> > +    def get_diff(self, top_level, filename):
> > +        return subprocess.run(['git', 'diff', '%s~..%s' % (self.commit,
> > self.commit),
> > +                               '--', '%s/%s' % (top_level, filename)],
> > +                              stdout=subprocess.PIPE).stdout.decode('utf-
> > 8')
> > +
> > +    def get_file(self, filename):
> > +        return subprocess.run(['git', 'show', '%s:%s' % (self.commit,
> > filename)],
> > +                              stdout=subprocess.PIPE).stdout.decode('utf-
> > 8')
> > +
> > +
> >  def check_file(top_level, commit, filename):
> >      # Extract the line numbers touched by the commit.
> > -    diff = subprocess.run(['git', 'diff', '%s~..%s' % (commit, commit), '
> > --',
> > -                           '%s/%s' % (top_level, filename)],
> > -                          stdout=subprocess.PIPE).stdout
> > -    diff = diff.decode('utf-8').splitlines(True)
> > +    diff = commit.get_diff(top_level, filename)
> > +    diff = diff.splitlines(True)
> >      commit_diff = parse_diff(diff)
> >  
> >      lines = []
> > @@ -476,9 +498,7 @@ def check_file(top_level, commit, filename):
> >  
> >      # Format the file after the commit with all formatters and compute the
> > diff
> >      # between the unformatted and formatted contents.
> > -    after = subprocess.run(['git', 'show', '%s:%s' % (commit, filename)],
> > -                           stdout=subprocess.PIPE).stdout
> > -    after = after.decode('utf-8')
> > +    after = commit.get_file(filename)
> >  
> >      formatted = after
> >      for formatter in Formatter.formatters(filename):
> > @@ -522,12 +542,7 @@ def check_file(top_level, commit, filename):
> >  
> >  
> >  def check_style(top_level, commit):
> > -    # Get the commit title and list of files.
> > -    ret = subprocess.run(['git', 'show', '--pretty=oneline','--name-only',
> > commit],
> > -                         stdout=subprocess.PIPE)
> > -    files = ret.stdout.decode('utf-8').splitlines()
> > -    title = files[0]
> > -    files = files[1:]
> > +    title, files = commit.get_info(top_level)
> >  
> >      separator = '-' * len(title)
> >      print(separator)
> > @@ -576,7 +591,7 @@ def extract_revlist(revs):
> >          revlist = ret.stdout.decode('utf-8').splitlines()
> >          revlist.reverse()
> >  
> > -    return revlist
> > +    return [Commit(x) for x in revlist]
> 
> As you're returning commits, should this function be renamed to
> extract_commits ?
> 
> With these issues addressed,
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> >  
> >  
> >  def git_top_level():
Laurent Pinchart Jan. 17, 2020, 10:34 p.m. UTC | #4 
Hi Nicolas,

On Fri, Jan 17, 2020 at 05:32:54PM -0500, Nicolas Dufresne wrote:
> Le samedi 18 janvier 2020 à 00:12 +0200, Laurent Pinchart a écrit :
> > On Fri, Jan 17, 2020 at 02:17:30PM -0500, Nicolas Dufresne wrote:
> > > From: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > > 
> > > This introduce a Commit class used in the final revlist list. All the
> > > git command are moved into that class. This class will be used to
> > > introduce new type of commit (index and amendment) needed to implement
> > > pre-commit hook support.
> > > 
> > > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > > ---
> > >  utils/checkstyle.py | 43 +++++++++++++++++++++++++++++--------------
> > >  1 file changed, 29 insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/utils/checkstyle.py b/utils/checkstyle.py
> > > index e7375b3..fb865c8 100644
> > > --- a/utils/checkstyle.py
> > > +++ b/utils/checkstyle.py
> > > @@ -458,12 +458,34 @@ class StripTrailingSpaceFormatter(Formatter):
> > >  # Style checking
> > >  #
> > >  
> > > +class Commit:
> > > +    commit = None
> > 
> > This isn't needed. This field would be accessed through Commit.commit,
> > which isn't equivalent to self.commit below.
> 
> Oops, you are right. I'm rusty in python.
> 
> > > +
> > > +    def __init__(self, commit):
> > > +        self.commit = commit
> > > +
> > > +    def get_info(self, top_level):
> > > +        # Get the commit title and list of files.
> > > +        ret = subprocess.run(['git', 'show', '--pretty=oneline', '--name-
> > > only',
> > > +                              self.commit],
> > > +                             stdout=subprocess.PIPE).stdout.decode('utf-8')
> > > +        files = ret.splitlines()
> > > +        return files[0], files[1:]
> > 
> > How about
> > 
> > 	ret = ret.splitlines()
> > 	title = ret[0]
> > 	files = ret[1:]
> > 	return title, files
> > 
> > to show what we're returning ?
> 
> I'd use a comment for that instead (it's interpreted language after all), what
> about:
> 
>   # returning title and files list as a tuple

With s/returning/Returning/ it works for me.

> > > +
> > > +    def get_diff(self, top_level, filename):
> > > +        return subprocess.run(['git', 'diff', '%s~..%s' % (self.commit,
> > > self.commit),
> > > +                               '--', '%s/%s' % (top_level, filename)],
> > > +                              stdout=subprocess.PIPE).stdout.decode('utf-
> > > 8')
> > > +
> > > +    def get_file(self, filename):
> > > +        return subprocess.run(['git', 'show', '%s:%s' % (self.commit,
> > > filename)],
> > > +                              stdout=subprocess.PIPE).stdout.decode('utf-
> > > 8')
> > > +
> > > +
> > >  def check_file(top_level, commit, filename):
> > >      # Extract the line numbers touched by the commit.
> > > -    diff = subprocess.run(['git', 'diff', '%s~..%s' % (commit, commit), '
> > > --',
> > > -                           '%s/%s' % (top_level, filename)],
> > > -                          stdout=subprocess.PIPE).stdout
> > > -    diff = diff.decode('utf-8').splitlines(True)
> > > +    diff = commit.get_diff(top_level, filename)
> > > +    diff = diff.splitlines(True)
> > >      commit_diff = parse_diff(diff)
> > >  
> > >      lines = []
> > > @@ -476,9 +498,7 @@ def check_file(top_level, commit, filename):
> > >  
> > >      # Format the file after the commit with all formatters and compute the
> > > diff
> > >      # between the unformatted and formatted contents.
> > > -    after = subprocess.run(['git', 'show', '%s:%s' % (commit, filename)],
> > > -                           stdout=subprocess.PIPE).stdout
> > > -    after = after.decode('utf-8')
> > > +    after = commit.get_file(filename)
> > >  
> > >      formatted = after
> > >      for formatter in Formatter.formatters(filename):
> > > @@ -522,12 +542,7 @@ def check_file(top_level, commit, filename):
> > >  
> > >  
> > >  def check_style(top_level, commit):
> > > -    # Get the commit title and list of files.
> > > -    ret = subprocess.run(['git', 'show', '--pretty=oneline','--name-only',
> > > commit],
> > > -                         stdout=subprocess.PIPE)
> > > -    files = ret.stdout.decode('utf-8').splitlines()
> > > -    title = files[0]
> > > -    files = files[1:]
> > > +    title, files = commit.get_info(top_level)
> > >  
> > >      separator = '-' * len(title)
> > >      print(separator)
> > > @@ -576,7 +591,7 @@ def extract_revlist(revs):
> > >          revlist = ret.stdout.decode('utf-8').splitlines()
> > >          revlist.reverse()
> > >  
> > > -    return revlist
> > > +    return [Commit(x) for x in revlist]
> > 
> > As you're returning commits, should this function be renamed to
> > extract_commits ?
> > 
> > With these issues addressed,
> > 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> > >  
> > >  
> > >  def git_top_level():

Patch
diff mbox series 

diff --git a/utils/checkstyle.py b/utils/checkstyle.py
index e7375b3..fb865c8 100644
--- a/utils/checkstyle.py
+++ b/utils/checkstyle.py
@@ -458,12 +458,34 @@  class StripTrailingSpaceFormatter(Formatter):
 # Style checking
 #
 
+class Commit:
+    commit = None
+
+    def __init__(self, commit):
+        self.commit = commit
+
+    def get_info(self, top_level):
+        # Get the commit title and list of files.
+        ret = subprocess.run(['git', 'show', '--pretty=oneline', '--name-only',
+                              self.commit],
+                             stdout=subprocess.PIPE).stdout.decode('utf-8')
+        files = ret.splitlines()
+        return files[0], files[1:]
+
+    def get_diff(self, top_level, filename):
+        return subprocess.run(['git', 'diff', '%s~..%s' % (self.commit, self.commit),
+                               '--', '%s/%s' % (top_level, filename)],
+                              stdout=subprocess.PIPE).stdout.decode('utf-8')
+
+    def get_file(self, filename):
+        return subprocess.run(['git', 'show', '%s:%s' % (self.commit, filename)],
+                              stdout=subprocess.PIPE).stdout.decode('utf-8')
+
+
 def check_file(top_level, commit, filename):
     # Extract the line numbers touched by the commit.
-    diff = subprocess.run(['git', 'diff', '%s~..%s' % (commit, commit), '--',
-                           '%s/%s' % (top_level, filename)],
-                          stdout=subprocess.PIPE).stdout
-    diff = diff.decode('utf-8').splitlines(True)
+    diff = commit.get_diff(top_level, filename)
+    diff = diff.splitlines(True)
     commit_diff = parse_diff(diff)
 
     lines = []
@@ -476,9 +498,7 @@  def check_file(top_level, commit, filename):
 
     # Format the file after the commit with all formatters and compute the diff
     # between the unformatted and formatted contents.
-    after = subprocess.run(['git', 'show', '%s:%s' % (commit, filename)],
-                           stdout=subprocess.PIPE).stdout
-    after = after.decode('utf-8')
+    after = commit.get_file(filename)
 
     formatted = after
     for formatter in Formatter.formatters(filename):
@@ -522,12 +542,7 @@  def check_file(top_level, commit, filename):
 
 
 def check_style(top_level, commit):
-    # Get the commit title and list of files.
-    ret = subprocess.run(['git', 'show', '--pretty=oneline','--name-only', commit],
-                         stdout=subprocess.PIPE)
-    files = ret.stdout.decode('utf-8').splitlines()
-    title = files[0]
-    files = files[1:]
+    title, files = commit.get_info(top_level)
 
     separator = '-' * len(title)
     print(separator)
@@ -576,7 +591,7 @@  def extract_revlist(revs):
         revlist = ret.stdout.decode('utf-8').splitlines()
         revlist.reverse()
 
-    return revlist
+    return [Commit(x) for x in revlist]
 
 
 def git_top_level():