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

Thanks - applied. I added a Makefile rule for running the check (make
check-pep8), but haven't included it in "make check" yet as it's failing at
present.

On Mon, Jan 29, 2018 at 10:36 AM, Murtuza Zabuawala <
murtuza(dot)zabuawala(at)enterprisedb(dot)com> wrote:

> Hi Dave,
>
> PFA updated patch.
>
> On Mon, Jan 29, 2018 at 3:23 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:
>
>> 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)?
>>
>
> ​Yes, we can add but I thought that it would be good if we have all the
> requirements at one place instead new developers/contributors having to go
> and search into separate directory for development/test related
> dependencies.
>
>
>>
>>
>>>
>>>
>>>> - 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
>>
>
>

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

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