[1/2] utils: checkstyle.py: Add author property to Commit class
diff mbox series

Message ID 20240805174807.10125-1-laurent.pinchart@ideasonboard.com
State Accepted
Headers show
Series
  • [1/2] utils: checkstyle.py: Add author property to Commit class
Related show

Commit Message

Laurent Pinchart Aug. 5, 2024, 5:48 p.m. UTC 
Extend the Commit class with an author property, retrieved from the
commit. It will be used to extend checkers.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 utils/checkstyle.py | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)


base-commit: 8af95d6854889dc66746429ccf8888e3a81f6baf

Comments

Stefan Klug Aug. 6, 2024, 7:05 a.m. UTC | #1 
Hi Laurent,

Thank you for the patch.

On Mon, Aug 05, 2024 at 08:48:06PM +0300, Laurent Pinchart wrote:
> Extend the Commit class with an author property, retrieved from the
> commit. It will be used to extend checkers.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  utils/checkstyle.py | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/utils/checkstyle.py b/utils/checkstyle.py
> index 7d480bdf4a2f..1eeb3809f7fe 100755
> --- a/utils/checkstyle.py
> +++ b/utils/checkstyle.py
> @@ -216,7 +216,7 @@ class Commit:
>          self._parse()
>  
>      def _parse_trailers(self, lines):
> -        for index in range(1, len(lines)):
> +        for index in range(2, len(lines)):
>              line = lines[index]
>              if not line:
>                  break
> @@ -227,12 +227,13 @@ class Commit:
>  
>      def _parse(self):
>          # Get the commit title and list of files.
> -        ret = subprocess.run(['git', 'show', '--format=%s%n%(trailers:only,unfold)', '--name-status',
> +        ret = subprocess.run(['git', 'show', '--format=%an <%ae>%n%s%n%(trailers:only,unfold)', '--name-status',

Nit: This line starts to be quite long...



>                                self.commit],
>                               stdout=subprocess.PIPE).stdout.decode('utf-8')
>          lines = ret.splitlines()
>  
> -        self._title = lines[0]
> +        self._author = lines[0]
> +        self._title = lines[1]
>  
>          index = self._parse_trailers(lines)
>          self._files = [CommitFile(f) for f in lines[index:] if f]
> @@ -240,6 +241,10 @@ class Commit:
>      def files(self, filter='AMR'):
>          return [f.filename for f in self._files if f.status in filter]
>  
> +    @property
> +    def author(self):
> +        return self._author
> +
>      @property
>      def title(self):
>          return self._title
> @@ -282,12 +287,13 @@ class Amendment(Commit):
>  
>      def _parse(self):
>          # Create a title using HEAD commit and parse the trailers.
> -        ret = subprocess.run(['git', 'show', '--format=%H %s%n%(trailers:only,unfold)',
> +        ret = subprocess.run(['git', 'show', '--format=%an <%ae>%n%H %s%n%(trailers:only,unfold)',
>                               '--no-patch'],
>                               stdout=subprocess.PIPE).stdout.decode('utf-8')
>          lines = ret.splitlines()
>  
> -        self._title = 'Amendment of ' + lines[0].strip()
> +        self._author = lines[0]
> +        self._title = 'Amendment of ' + lines[1].strip()

This raises the question why the title needs stripping but not the
author.

Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com> 

Cheers,
Stefan


>  
>          self._parse_trailers(lines)
>  
> 
> base-commit: 8af95d6854889dc66746429ccf8888e3a81f6baf
> -- 
> Regards,
> 
> Laurent Pinchart
>
Laurent Pinchart Aug. 6, 2024, 9:02 a.m. UTC | #2 
On Tue, Aug 06, 2024 at 09:05:04AM +0200, Stefan Klug wrote:
> Hi Laurent,
> 
> Thank you for the patch.
> 
> On Mon, Aug 05, 2024 at 08:48:06PM +0300, Laurent Pinchart wrote:
> > Extend the Commit class with an author property, retrieved from the
> > commit. It will be used to extend checkers.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  utils/checkstyle.py | 16 +++++++++++-----
> >  1 file changed, 11 insertions(+), 5 deletions(-)
> > 
> > diff --git a/utils/checkstyle.py b/utils/checkstyle.py
> > index 7d480bdf4a2f..1eeb3809f7fe 100755
> > --- a/utils/checkstyle.py
> > +++ b/utils/checkstyle.py
> > @@ -216,7 +216,7 @@ class Commit:
> >          self._parse()
> >  
> >      def _parse_trailers(self, lines):
> > -        for index in range(1, len(lines)):
> > +        for index in range(2, len(lines)):
> >              line = lines[index]
> >              if not line:
> >                  break
> > @@ -227,12 +227,13 @@ class Commit:
> >  
> >      def _parse(self):
> >          # Get the commit title and list of files.
> > -        ret = subprocess.run(['git', 'show', '--format=%s%n%(trailers:only,unfold)', '--name-status',
> > +        ret = subprocess.run(['git', 'show', '--format=%an <%ae>%n%s%n%(trailers:only,unfold)', '--name-status',
> 
> Nit: This line starts to be quite long...

I'll wrap it.

> >                                self.commit],
> >                               stdout=subprocess.PIPE).stdout.decode('utf-8')
> >          lines = ret.splitlines()
> >  
> > -        self._title = lines[0]
> > +        self._author = lines[0]
> > +        self._title = lines[1]
> >  
> >          index = self._parse_trailers(lines)
> >          self._files = [CommitFile(f) for f in lines[index:] if f]
> > @@ -240,6 +241,10 @@ class Commit:
> >      def files(self, filter='AMR'):
> >          return [f.filename for f in self._files if f.status in filter]
> >  
> > +    @property
> > +    def author(self):
> > +        return self._author
> > +
> >      @property
> >      def title(self):
> >          return self._title
> > @@ -282,12 +287,13 @@ class Amendment(Commit):
> >  
> >      def _parse(self):
> >          # Create a title using HEAD commit and parse the trailers.
> > -        ret = subprocess.run(['git', 'show', '--format=%H %s%n%(trailers:only,unfold)',
> > +        ret = subprocess.run(['git', 'show', '--format=%an <%ae>%n%H %s%n%(trailers:only,unfold)',
> >                               '--no-patch'],
> >                               stdout=subprocess.PIPE).stdout.decode('utf-8')
> >          lines = ret.splitlines()
> >  
> > -        self._title = 'Amendment of ' + lines[0].strip()
> > +        self._author = lines[0]
> > +        self._title = 'Amendment of ' + lines[1].strip()
> 
> This raises the question why the title needs stripping but not the
> author.

I asked myself the same question and didn't orignally investigate. Now
that I have, I think I can drop the strip. It was originally added in
commit 17b3c794095e ("checkstyle: Add support for checking style on
amendments"):

        ret = subprocess.run(['git', 'show', '--pretty=oneline', '--no-patch'],
                             stdout=subprocess.PIPE).stdout.decode('utf-8')
        title = 'Amendment of ' + ret.strip()

There the strip was needed to drop the newline character. Then commit
4694e441c313 ("utils: checkstyle.py: Extract title and trailers with one
command") modified the code to

        ret = subprocess.run(['git', 'show', '--format=%H %s%n%(trailers:only,unf

                             '--no-patch'],
                             stdout=subprocess.PIPE).stdout.decode('utf-8')
        lines = ret.splitlines()

        self._title = 'Amendment of ' + lines[0].strip()

The splitlines() call removes the new line characters, but the strip()
was kept. I'll drop it in v2.

> Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com> 
> 
> >  
> >          self._parse_trailers(lines)
> >  
> > 
> > base-commit: 8af95d6854889dc66746429ccf8888e3a81f6baf
Kieran Bingham Aug. 7, 2024, 10:20 a.m. UTC | #3 
Quoting Laurent Pinchart (2024-08-06 10:02:01)
> On Tue, Aug 06, 2024 at 09:05:04AM +0200, Stefan Klug wrote:
> > Hi Laurent,
> > 
> > Thank you for the patch.
> > 
> > On Mon, Aug 05, 2024 at 08:48:06PM +0300, Laurent Pinchart wrote:
> > > Extend the Commit class with an author property, retrieved from the
> > > commit. It will be used to extend checkers.
> > > 
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > >  utils/checkstyle.py | 16 +++++++++++-----
> > >  1 file changed, 11 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/utils/checkstyle.py b/utils/checkstyle.py
> > > index 7d480bdf4a2f..1eeb3809f7fe 100755
> > > --- a/utils/checkstyle.py
> > > +++ b/utils/checkstyle.py
> > > @@ -216,7 +216,7 @@ class Commit:
> > >          self._parse()
> > >  
> > >      def _parse_trailers(self, lines):
> > > -        for index in range(1, len(lines)):
> > > +        for index in range(2, len(lines)):
> > >              line = lines[index]
> > >              if not line:
> > >                  break
> > > @@ -227,12 +227,13 @@ class Commit:
> > >  
> > >      def _parse(self):
> > >          # Get the commit title and list of files.
> > > -        ret = subprocess.run(['git', 'show', '--format=%s%n%(trailers:only,unfold)', '--name-status',
> > > +        ret = subprocess.run(['git', 'show', '--format=%an <%ae>%n%s%n%(trailers:only,unfold)', '--name-status',
> > 
> > Nit: This line starts to be quite long...
> 
> I'll wrap it.
> 
> > >                                self.commit],
> > >                               stdout=subprocess.PIPE).stdout.decode('utf-8')
> > >          lines = ret.splitlines()
> > >  
> > > -        self._title = lines[0]
> > > +        self._author = lines[0]
> > > +        self._title = lines[1]
> > >  
> > >          index = self._parse_trailers(lines)
> > >          self._files = [CommitFile(f) for f in lines[index:] if f]
> > > @@ -240,6 +241,10 @@ class Commit:
> > >      def files(self, filter='AMR'):
> > >          return [f.filename for f in self._files if f.status in filter]
> > >  
> > > +    @property
> > > +    def author(self):
> > > +        return self._author
> > > +
> > >      @property
> > >      def title(self):
> > >          return self._title
> > > @@ -282,12 +287,13 @@ class Amendment(Commit):
> > >  
> > >      def _parse(self):
> > >          # Create a title using HEAD commit and parse the trailers.
> > > -        ret = subprocess.run(['git', 'show', '--format=%H %s%n%(trailers:only,unfold)',
> > > +        ret = subprocess.run(['git', 'show', '--format=%an <%ae>%n%H %s%n%(trailers:only,unfold)',
> > >                               '--no-patch'],
> > >                               stdout=subprocess.PIPE).stdout.decode('utf-8')
> > >          lines = ret.splitlines()
> > >  
> > > -        self._title = 'Amendment of ' + lines[0].strip()
> > > +        self._author = lines[0]
> > > +        self._title = 'Amendment of ' + lines[1].strip()
> > 
> > This raises the question why the title needs stripping but not the
> > author.
> 
> I asked myself the same question and didn't orignally investigate. Now
> that I have, I think I can drop the strip. It was originally added in
> commit 17b3c794095e ("checkstyle: Add support for checking style on
> amendments"):
> 
>         ret = subprocess.run(['git', 'show', '--pretty=oneline', '--no-patch'],
>                              stdout=subprocess.PIPE).stdout.decode('utf-8')
>         title = 'Amendment of ' + ret.strip()
> 
> There the strip was needed to drop the newline character. Then commit
> 4694e441c313 ("utils: checkstyle.py: Extract title and trailers with one
> command") modified the code to
> 
>         ret = subprocess.run(['git', 'show', '--format=%H %s%n%(trailers:only,unf
> 
>                              '--no-patch'],
>                              stdout=subprocess.PIPE).stdout.decode('utf-8')
>         lines = ret.splitlines()
> 
>         self._title = 'Amendment of ' + lines[0].strip()
> 
> The splitlines() call removes the new line characters, but the strip()
> was kept. I'll drop it in v2.

I'm fine with all that.

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> 
> > Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com> 
> > 
> > >  
> > >          self._parse_trailers(lines)
> > >  
> > > 
> > > base-commit: 8af95d6854889dc66746429ccf8888e3a81f6baf
> 
> -- 
> Regards,
> 
> Laurent Pinchart

Patch
diff mbox series 

diff --git a/utils/checkstyle.py b/utils/checkstyle.py
index 7d480bdf4a2f..1eeb3809f7fe 100755
--- a/utils/checkstyle.py
+++ b/utils/checkstyle.py
@@ -216,7 +216,7 @@  class Commit:
         self._parse()
 
     def _parse_trailers(self, lines):
-        for index in range(1, len(lines)):
+        for index in range(2, len(lines)):
             line = lines[index]
             if not line:
                 break
@@ -227,12 +227,13 @@  class Commit:
 
     def _parse(self):
         # Get the commit title and list of files.
-        ret = subprocess.run(['git', 'show', '--format=%s%n%(trailers:only,unfold)', '--name-status',
+        ret = subprocess.run(['git', 'show', '--format=%an <%ae>%n%s%n%(trailers:only,unfold)', '--name-status',
                               self.commit],
                              stdout=subprocess.PIPE).stdout.decode('utf-8')
         lines = ret.splitlines()
 
-        self._title = lines[0]
+        self._author = lines[0]
+        self._title = lines[1]
 
         index = self._parse_trailers(lines)
         self._files = [CommitFile(f) for f in lines[index:] if f]
@@ -240,6 +241,10 @@  class Commit:
     def files(self, filter='AMR'):
         return [f.filename for f in self._files if f.status in filter]
 
+    @property
+    def author(self):
+        return self._author
+
     @property
     def title(self):
         return self._title
@@ -282,12 +287,13 @@  class Amendment(Commit):
 
     def _parse(self):
         # Create a title using HEAD commit and parse the trailers.
-        ret = subprocess.run(['git', 'show', '--format=%H %s%n%(trailers:only,unfold)',
+        ret = subprocess.run(['git', 'show', '--format=%an <%ae>%n%H %s%n%(trailers:only,unfold)',
                              '--no-patch'],
                              stdout=subprocess.PIPE).stdout.decode('utf-8')
         lines = ret.splitlines()
 
-        self._title = 'Amendment of ' + lines[0].strip()
+        self._author = lines[0]
+        self._title = 'Amendment of ' + lines[1].strip()
 
         self._parse_trailers(lines)