[libcamera-devel,1/2] utils: checkstyle: Add formatter to sort #include statements

3239 diff mbox series

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

Hi Laurent,

On 22/03/2020 12:02, Laurent Pinchart wrote:

Ha, I love this :-)

One less thing to worry about in reviews!

> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  utils/checkstyle.py | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/utils/checkstyle.py b/utils/checkstyle.py
> index 0827a1e6ba0f..5fd63f047781 100755
> --- a/utils/checkstyle.py
> +++ b/utils/checkstyle.py
> @@ -481,6 +481,39 @@ class DoxygenFormatter(Formatter):
>          return '\n'.join(lines)
>  
>  
> +class IncludeOrderFormatter(Formatter):
> +    patterns = ('*.cpp', '*.h')
> +
> +    include_regex = re.compile('^#include ["<]([^">]*)[">]')
> +
> +    @classmethod
> +    def format(cls, filename, data):
> +        lines = []
> +        includes = []
> +
> +        for line in data.split('\n'):
> +            match = IncludeOrderFormatter.include_regex.match(line)
> +            if match:
> +                includes.append((line, match.group(1)))
> +                continue
> +

Maybe it's just me, but this seems somehow quite hard to interpret and I
found it difficult to work out /how/ the code is parsing the blocks.

I'm not yet sure what, but a comment describing what happens somewhere
would be useful for the future me...

(It didn't help that I missed the continue on my first read, so I didn't
realise the code below was only executed for non #include lines).

Maybe a top level comment saying:
 # Parse blocks of #include statements, and output them as a sorted list
 # when we reach a non #include statement.


Perhaps that's verging towards commenting what the code does rather than
why, but now that I 'know' that information, it's easier to parse the
paths of the code to determine the 'how'...


> +            if len(includes):
> +                includes.sort(key=lambda i: i[1])
> +                for include in includes:
> +                    lines.append(include[0])
> +                includes = []
> +
> +            lines.append(line)
> +
> +        if len(includes):
> +            includes.sort(key=lambda i: i[1])
> +            for include in includes:
> +                lines.append(include[0])
> +            includes = []
> +
> +        return '\n'.join(lines)
> +
> +
>  class StripTrailingSpaceFormatter(Formatter):
>      patterns = ('*.c', '*.cpp', '*.h', '*.py', 'meson.build')
>  
>

Hi Kieran,

On Mon, Mar 23, 2020 at 11:35:39AM +0000, Kieran Bingham wrote:
> On 22/03/2020 12:02, Laurent Pinchart wrote:
> 
> Ha, I love this :-)
> 
> One less thing to worry about in reviews!
> 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  utils/checkstyle.py | 33 +++++++++++++++++++++++++++++++++
> >  1 file changed, 33 insertions(+)
> > 
> > diff --git a/utils/checkstyle.py b/utils/checkstyle.py
> > index 0827a1e6ba0f..5fd63f047781 100755
> > --- a/utils/checkstyle.py
> > +++ b/utils/checkstyle.py
> > @@ -481,6 +481,39 @@ class DoxygenFormatter(Formatter):
> >          return '\n'.join(lines)
> >  
> >  
> > +class IncludeOrderFormatter(Formatter):
> > +    patterns = ('*.cpp', '*.h')
> > +
> > +    include_regex = re.compile('^#include ["<]([^">]*)[">]')
> > +
> > +    @classmethod
> > +    def format(cls, filename, data):
> > +        lines = []
> > +        includes = []
> > +
> > +        for line in data.split('\n'):
> > +            match = IncludeOrderFormatter.include_regex.match(line)
> > +            if match:
> > +                includes.append((line, match.group(1)))
> > +                continue
> > +
> 
> Maybe it's just me, but this seems somehow quite hard to interpret and I
> found it difficult to work out /how/ the code is parsing the blocks.
> 
> I'm not yet sure what, but a comment describing what happens somewhere
> would be useful for the future me...
> 
> (It didn't help that I missed the continue on my first read, so I didn't
> realise the code below was only executed for non #include lines).
> 
> Maybe a top level comment saying:
>  # Parse blocks of #include statements, and output them as a sorted list
>  # when we reach a non #include statement.
> 
> 
> Perhaps that's verging towards commenting what the code does rather than
> why, but now that I 'know' that information, it's easier to parse the
> paths of the code to determine the 'how'...

diff --git a/utils/checkstyle.py b/utils/checkstyle.py
index 5fd63f047781..b594a19aed5b 100755
--- a/utils/checkstyle.py
+++ b/utils/checkstyle.py
@@ -491,12 +491,18 @@ class IncludeOrderFormatter(Formatter):
         lines = []
         includes = []

+        # Parse blocks of #include statements, and output them as a sorted list
+        # when we reach a non #include statement.
         for line in data.split('\n'):
             match = IncludeOrderFormatter.include_regex.match(line)
             if match:
+                # If the current line is an #include statement, add it to the
+                # includes group and continue to the next line.
                 includes.append((line, match.group(1)))
                 continue

+            # The current line is not an #include statement, output the sorted
+            # stashed includes first, and then the current line.
             if len(includes):
                 includes.sort(key=lambda i: i[1])
                 for include in includes:
@@ -505,6 +511,8 @@ class IncludeOrderFormatter(Formatter):

             lines.append(line)

+        # In the unlikely case the file ends with an #include statement, make
+        # sure we output the stashed includes.
         if len(includes):
             includes.sort(key=lambda i: i[1])
             for include in includes:

Does this look good to you ? If so, your Rb tag will be appreciated (for
the result of squashing the original patch with this change) :-)

> > +            if len(includes):
> > +                includes.sort(key=lambda i: i[1])
> > +                for include in includes:
> > +                    lines.append(include[0])
> > +                includes = []
> > +
> > +            lines.append(line)
> > +
> > +        if len(includes):
> > +            includes.sort(key=lambda i: i[1])
> > +            for include in includes:
> > +                lines.append(include[0])
> > +            includes = []
> > +
> > +        return '\n'.join(lines)
> > +
> > +
> >  class StripTrailingSpaceFormatter(Formatter):
> >      patterns = ('*.c', '*.cpp', '*.h', '*.py', 'meson.build')
> >  
> >

Hi Laurent,

On 23/03/2020 11:41, Laurent Pinchart wrote:
> Hi Kieran,
> 
> On Mon, Mar 23, 2020 at 11:35:39AM +0000, Kieran Bingham wrote:
>> On 22/03/2020 12:02, Laurent Pinchart wrote:
>>
>> Ha, I love this :-)
>>
>> One less thing to worry about in reviews!
>>
>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>> ---
>>>  utils/checkstyle.py | 33 +++++++++++++++++++++++++++++++++
>>>  1 file changed, 33 insertions(+)
>>>
>>> diff --git a/utils/checkstyle.py b/utils/checkstyle.py
>>> index 0827a1e6ba0f..5fd63f047781 100755
>>> --- a/utils/checkstyle.py
>>> +++ b/utils/checkstyle.py
>>> @@ -481,6 +481,39 @@ class DoxygenFormatter(Formatter):
>>>          return '\n'.join(lines)
>>>  
>>>  
>>> +class IncludeOrderFormatter(Formatter):
>>> +    patterns = ('*.cpp', '*.h')
>>> +
>>> +    include_regex = re.compile('^#include ["<]([^">]*)[">]')
>>> +
>>> +    @classmethod
>>> +    def format(cls, filename, data):
>>> +        lines = []
>>> +        includes = []
>>> +
>>> +        for line in data.split('\n'):
>>> +            match = IncludeOrderFormatter.include_regex.match(line)
>>> +            if match:
>>> +                includes.append((line, match.group(1)))
>>> +                continue
>>> +
>>
>> Maybe it's just me, but this seems somehow quite hard to interpret and I
>> found it difficult to work out /how/ the code is parsing the blocks.
>>
>> I'm not yet sure what, but a comment describing what happens somewhere
>> would be useful for the future me...
>>
>> (It didn't help that I missed the continue on my first read, so I didn't
>> realise the code below was only executed for non #include lines).
>>
>> Maybe a top level comment saying:
>>  # Parse blocks of #include statements, and output them as a sorted list
>>  # when we reach a non #include statement.
>>
>>
>> Perhaps that's verging towards commenting what the code does rather than
>> why, but now that I 'know' that information, it's easier to parse the
>> paths of the code to determine the 'how'...
> 
> diff --git a/utils/checkstyle.py b/utils/checkstyle.py
> index 5fd63f047781..b594a19aed5b 100755
> --- a/utils/checkstyle.py
> +++ b/utils/checkstyle.py
> @@ -491,12 +491,18 @@ class IncludeOrderFormatter(Formatter):
>          lines = []
>          includes = []
> 
> +        # Parse blocks of #include statements, and output them as a sorted list
> +        # when we reach a non #include statement.
>          for line in data.split('\n'):
>              match = IncludeOrderFormatter.include_regex.match(line)
>              if match:
> +                # If the current line is an #include statement, add it to the
> +                # includes group and continue to the next line.
>                  includes.append((line, match.group(1)))
>                  continue
> 
> +            # The current line is not an #include statement, output the sorted
> +            # stashed includes first, and then the current line.
>              if len(includes):
>                  includes.sort(key=lambda i: i[1])
>                  for include in includes:
> @@ -505,6 +511,8 @@ class IncludeOrderFormatter(Formatter):
> 
>              lines.append(line)
> 
> +        # In the unlikely case the file ends with an #include statement, make
> +        # sure we output the stashed includes.
>          if len(includes):
>              includes.sort(key=lambda i: i[1])
>              for include in includes:
> 
> Does this look good to you ? If so, your Rb tag will be appreciated (for
> the result of squashing the original patch with this change) :-)

Haha - more than I was expecting -  but that's helpful in this case I
think :-)

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

> 
>>> +            if len(includes):
>>> +                includes.sort(key=lambda i: i[1])
>>> +                for include in includes:
>>> +                    lines.append(include[0])
>>> +                includes = []
>>> +
>>> +            lines.append(line)
>>> +
>>> +        if len(includes):
>>> +            includes.sort(key=lambda i: i[1])
>>> +            for include in includes:
>>> +                lines.append(include[0])
>>> +            includes = []
>>> +
>>> +        return '\n'.join(lines)
>>> +
>>> +
>>>  class StripTrailingSpaceFormatter(Formatter):
>>>      patterns = ('*.c', '*.cpp', '*.h', '*.py', 'meson.build')
>>>  
>>>
>