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 < [email protected]> wrote: > Hi Dave, > > PFA updated patch. > > On Mon, Jan 29, 2018 at 3:23 PM, Dave Page <[email protected]> wrote: > >> Hi >> >> On Sun, Jan 28, 2018 at 4:42 AM, Murtuza Zabuawala < >> [email protected]> wrote: >> >>> Hi Dave, >>> >>> On Fri, Jan 26, 2018 at 10:33 PM, Dave Page <[email protected]> wrote: >>> >>>> Hi >>>> >>>> On Fri, Jan 26, 2018 at 1:26 PM, Murtuza Zabuawala < >>>> [email protected]> 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
