[Piglit] [PATCH 18/26] parse_glspec.py: PEP8 compliance

[Chad Versace]

On 07/23/2013 07:46 AM, Dylan Baker wrote:
> I'll look through for the parens, It might be in a seperate patch, but I
> think it deserves to be here, since PEP8 is all about readability.

If you want to defer the parens to a separate patch, that's ok. But, in this
patch we shouldn't commit this atrocious dict. It's really unreadable.

 >>> +        return 'GLES{0}.{1}'.format(ones, tenths), {'kind': 'GLES',
 >>> +                                                    'gl_10x_version':
 >>> +                                                    10 * ones + tenths}


>
> The final one it complained about, but after reading PEP8 again I
> understand why, and can fix it. In this case it wants something like this:
> variable = {
>       key: value,
>       key: value
> }
>
> With the important note that the closing brace (or parentheses or bracket)
> should either be on the last line of text, or should be on the next line
> even with the indent of the first line.

Strange. PEP8 says

--- -8<- ---
The closing brace/bracket/parenthesis on multi-line constructs may either line up under the first non-whitespace 
character of the last line of list, as in:

my_list = [
     1, 2, 3,
     4, 5, 6,
     ]
result = some_function_that_takes_arguments(
     'a', 'b', 'c',
     'd', 'e', 'f',
     )
or it may be lined up under the first character of the line that starts the multi-line construct, as in:

my_list = [
     1, 2, 3,
     4, 5, 6,
]
result = some_function_that_takes_arguments(
     'a', 'b', 'c',
     'd', 'e', 'f',
)
--- >8- ---

So, the tool should not have complained about

my_dict = {
     k1: v1,
     k2: v2,
     }

Anyways, if the pep8 tool does incorrectly complain about that dict,
then go ahead and fix it. I'm just concerned that, even if I take
pains to code according to PEP8, then the tool will still loudly
complain.


> On Mon, Jul 22, 2013 at 2:26 PM, Chad Versace
> <chad.versace at linux.intel.com>wrote:
>
>> On 07/10/2013 03:19 PM, Dylan Baker wrote:
>>
>>> There are a 3 lines that are just over the recommended 79 character
>>> limit, however, breaking them made them harder to read so they were
>>> left.
>>> ---
>>>    glapi/parse_glspec.py | 76 ++++++++++++++++++++++++------**
>>> ---------------------
>>>    1 file changed, 36 insertions(+), 40 deletions(-)
>>>
>>
>>
>>
>>
>>>        m = GLES_VERSION_REGEXP.match(**category_name)
>>>        if m:
>>>            ones = int(m.group(1))
>>>            tenths = int(m.group(2))
>>> -        return 'GLES{0}.{1}'.format(ones, tenths), {
>>> -            'kind': 'GLES',
>>> -            'gl_10x_version': 10 * ones + tenths
>>> -            }
>>> +        return 'GLES{0}.{1}'.format(ones, tenths), {'kind': 'GLES',
>>> +                                                    'gl_10x_version':
>>> +                                                    10 * ones + tenths}
>>>
>>>
>>
>> The original was significantly more readable. Maybe you could achieve PEP-8
>> correctness while maintaining readability by using explicit parens around
>> the
>> tuple. Maybe like this:
>>
>>      return ('GLES{0}.{1}'.format(ones, tenths),
>>              {'kind': 'GLES',
>>               'gl_10x_version': 10 * ones + tenths})
>>
>> or this
>>
>>      return (
>>
>>          'GLES{0}.{1}'.format(ones, tenths),
>>          {'kind': 'GLES',
>>           'gl_10x_version': 10 * ones + tenths}
>>      )
>>
>>  From my interpretation of the PEP8 doc, both are PEP8 correct.
>>
>>
>>
>>
>>         extension_name = 'GL_' + category_name
>>> -    return extension_name, {
>>> -        'kind': 'extension',
>>> -        'extension_name': extension_name
>>> -        }
>>> +    return extension_name, {'kind': 'extension',
>>> +                            'extension_name': extension_name}
>>>
>>
>>
>> While your changing the formatting of tuples, could you add explicit
>> parens to aid readability? There are several other locations in this
>> patch where parens could be added. Or do you think that belongs to
>> a separate patch?
>>
>>
>>
>>
>>             if name not in self.functions:
>>> -            self.functions[name] = {
>>> -                'return_type': return_type,
>>> -                'param_names': param_names,
>>> -                'param_types': param_types,
>>> -                'categories': [category],
>>> -                }
>>> +            self.functions[name] = {'return_type': return_type,
>>> +                                    'param_names': param_names,
>>> +                                    'param_types': param_types,
>>> +                                    'categories': [category]}
>>>
>>
>> Did the PEP8 tool complain about this one?
>>
>>
>