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

Hi Dave,

PFA updated patch.

On Mon, Jan 29, 2018 at 3:23 PM, Dave Page <dpage@pgadmin.org> wrote:

Hi

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

Hi Dave,

On Fri, Jan 26, 2018 at 10:33 PM, Dave Page <dpage@pgadmin.org> wrote:

Hi

On Fri, Jan 26, 2018 at 1:26 PM, Murtuza Zabuawala <murtuza.zabuawala@enterprisedb.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 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 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