Re: [pgAdmin4][RM#3063] Add 'pycodestyle ' Python PEP-8 checker module

Hi

On Sun, Jan 28, 2018 at 4:42 AM, Murtuza Zabuawala <
murtuza(dot)zabuawala(at)enterprisedb(dot)com> wrote:

> Hi Dave,
>
> On Fri, Jan 26, 2018 at 10:33 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:
>
>> Hi
>>
>> On Fri, Jan 26, 2018 at 1:26 PM, Murtuza Zabuawala <
>> murtuza(dot)zabuawala(at)enterprisedb(dot)com> wrote:
>>
>>> Hi,
>>>
>>> PFA patch to add 'pycodestyle ' (formerly called pep8) which is a Python
>>> PEP-8 checker module.
>>>
>>
>> Nice!
>>
>> A couple of thoughts:
>>
>> - I think this should go into web/regression/requirements.txt.
>> Otherwise, it'll end up being installed in the packages we build.
>>
>
> ​I have come up with new design for requirements (Patch attached).
>
> We can have layered requirement files, f
> or example, we
> ​can have all the common dependancies in one file​
> ​say ​
> requirements
> /​
> common.tx
> ​t, then​
>
> ​we can have ​
> requirements
> /​
> ​
> requirements
> ​-​
> dev
> .tx
> ​t which will inherit all the common dependancies from
> ​
> common.tx
> ​t plus it has its own development related
> dependancies
> ​, similar with ​
> requirements
> /​
> ​
> requirements.tx
> t
> which will inherit all the common dependancies from
> ​
> common.tx
> ​t plus it has its own production related
> dependancies
> ​(if any),
>
> Then we can use pip install -r ​
> requirements
> ​/
> requirements
> ​-​
> dev.txt​
>
> ​for the development ​
> ​and
> ​
> pip install -r ​
> requirements
> ​/requirements.txt​
>
> ​for the production ​
> accordingly
> ​ without interfering between them.​
>

Can't we just add "-r ../../requirements.txt" to
web/regression/requirements.txt? I can't imagine any situation in which
we'd have a requirement for running that isn't needed for testing (or that
we'd care about if we did)?

>
>
>> - We should probably include a new Makefile target to run it, as well as
>> including it in the "check" target.
>>
> ​Sorry, I didn't get you on this one, could you please throw some light on
> this?​
>
>

We have targets in /Makefile for testing. We should add one for this, and
ensure it's also called when running "make check".

>
>
>> - Once we expect the output to be clean, I think we should add a runner
>> script to ci/ so we automate checking.
>>
> ​Yes, we should
> ​.
> ​
>
>> - Would a shorter output be more useful? In particular, it seems to be
>> outputting a paragraph of text every time if finds a line longer than 79
>> chars. Really, we just need the location of each issue, and then the
>> summary I think, e.g.
>>
>> ​Done.
> FYI you can configure it via show-source (displays faulty code) &
> show-pep8
> (displays
> ​corrective
>
> ​suggestions
> )
> ​ ​
> option
> ​s​
> in web/.pycodestyle
>

Right. That definitely looks more useful now.

>
> ...
>> ...
>> ./pgadmin/utils/tests/test_versioned_template_loader.py:127: [E501] line
>> too long (104 > 79 characters)
>> ./pgadmin/utils/tests/test_versioned_template_loader.py:118: [E501] line
>> too long (89 > 79 characters)
>> ./pgadmin/utils/tests/test_versioned_template_loader.py:116: [E501] line
>> too long (89 > 79 characters)
>> 3 E111 indentation is not a multiple of four
>> 52 E121 continuation line under-indented for hanging indent
>> 19 E122 continuation line missing indentation or outdented
>> 27 E123 closing bracket does not match indentation of opening
>> bracket's line
>> 3 E124 closing bracket does not match visual indentation
>> 6 E125 continuation line with same indent as next logical line
>> 79 E126 continuation line over-indented for hanging indent
>> 67 E127 continuation line over-indented for visual indent
>> 15 E128 continuation line under-indented for visual indent
>> 3 E129 visually indented line with same indent as next logical line
>> 11 E131 continuation line unaligned for hanging indent
>> 6 E203 whitespace before ','
>> 1 E211 whitespace before '['
>> 2 E221 multiple spaces before operator
>> 1 E222 multiple spaces after operator
>> 19 E225 missing whitespace around operator
>> 19 E226 missing whitespace around arithmetic operator
>> 16 E231 missing whitespace after ','
>> 2 E241 multiple spaces after ','
>> 7 E251 unexpected spaces around keyword / parameter equals
>> 1 E261 at least two spaces before inline comment
>> 1 E271
>> <https://maps.google.com/?q=1%C2%A0+%C2%A0+%C2%A0+%C2%A0E271&entry=gmail&source=g>
>> multiple spaces after keyword
>> 17 E302 expected 2 blank lines, found 1
>> 23 E303 too many blank lines (2)
>> 14 E305 expected 2 blank lines after class or function definition,
>> found 1
>> 1 E401 multiple imports on one line
>> 1176 E501 line too long (80 > 79 characters)
>> 15 E502 the backslash is redundant between brackets
>> 4 E703 statement ends with a semicolon
>> 8 E712
>> <https://maps.google.com/?q=8%C2%A0+%C2%A0+%C2%A0+%C2%A0E712&entry=gmail&source=g>
>> comparison to False should be 'if cond is False:' or 'if not cond:'
>> 3 E713 test for membership should be 'not in'
>> 21 E722 do not use bare except'
>> 1 E741 ambiguous variable name 'l'
>> 11 W391 blank line at end of file
>> 23 W503 line break before binary operator
>> 1677
>>
>> Thoughts?
>>
>
> ​Please review.
> ​
>
>>
>> --
>> Dave Page
>> Blog: http://pgsnake.blogspot.com
>> Twitter: @pgsnake
>>
>> EnterpriseDB UK: http://www.enterprisedb.com
>> The Enterprise PostgreSQL Company
>>
>
>

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company