Config files for auto linting #3264

humitos · 2017-11-15T03:51:22Z

autoflake, autopep8, docformatter, isort, yapf

There are some things that we need to define:

max columns
style of the docstrings
order of the sections of imports
way of split long import lines
etc

If you want to go deeper, you should read all the knobs from yapf (which is the most agressive here) and tweak all the options as you wish). For now, I think it's a good start.

I'm adding a couple of modified files after aplying these settings as examples so we can discuss a little on the automatic styling code and define if it worth.

autoflake, autopep8, docformatter, isort, yapf

humitos · 2017-11-15T04:02:34Z

I applied the auto styling to three files (views, models and urls). Please, take a look at them and let me know any commen like "I don't like this format as this" or "I like this split here". I will tell you if we can change that with a setting so we can tweak and adapt to our needings.

agjohnson

Looks promising.

What would workflow be? The developer would still be responsible for running this before pushing?

I really like how this could easily automate us getting up to strict linting. I already have my tooling set to use strict linting by default, and am generally testing linting before on each file inside vim -- though I generally don't run a tox -e lint step as CI will.

It doesn't seem like we can necessarily rely on this tooling to lint for us, but I haven't tried running these tools yet. We'd definitely need a wrapper script as we won't remember to run that block of commands before each PR. Currently, I view this more as a path for intermittent linting cleanup and a way to help automate bumping up the linting strictness, as the added steps likely won't be run manually. However, even if this is the case, if we can get to strict linting then CI will bring up issues with code quality, and it won't be an item that wastes review cycles -- definitely a win.

agjohnson · 2017-11-15T04:30:31Z

readthedocs/builds/urls.py

-"""URL configuration for builds app."""
+"""
+URL configuration for builds app.
+"""


According to pep257, this should be a single line:
https://www.python.org/dev/peps/pep-0257/#one-line-docstrings

Yes. I forgot to remove the hack in my config that I used for a non-standard proyect :)

--make-summaries-multilines does the trick

agjohnson · 2017-11-15T04:32:04Z

readthedocs/builds/urls.py

-    url(r'^(?P<project_slug>[-\w]+)/$',
-        builds_redirect_list,
-        name='old_builds_project_list'),
+    url(r'^(?P<project_slug>[-\w]+)/(?P<pk>\d+)/$', builds_redirect_detail, name='old_builds_detail'),


I prefer line length of 80. Our linting is set to max of 100, but tried to eventually get down to 80

I think that 80 is not enought, I liked working on a project that was 120. But anyway, my thoughts.

I we use 80, the tools will split it always at 80... it won't be lines with more columns. It's strict.

I'm fine with a strict 80, most of our code is wrapped at 80 already. We have set max length 100 for some of the lines that haven't been rewrapped, but have been refactoring those out. 80 is (close) to pep8 compliant, so it's expected. I have a bunch of personal reasons to like 80, but those are just mine 🙂

I won't ask why... :)

I just changed the 120 by 80 columns in all the places.

agjohnson · 2017-11-15T04:33:01Z

readthedocs/core/models.py

+class UserProfile(models.Model):
+    """
+    Additional information about a User.
+    """


Same note on single line docstring here, but modern suggestion is no space before docstring, 👍

Without the --make-summary-multi-line it will be

"""Additional information about a User."""

Since, I removed that argument, it will be as you like.

but modern suggestion is no space before docstring

What do you mean with that?

agjohnson · 2017-11-15T04:52:10Z

readthedocs/core/models.py

-                                    )
-    allow_email = models.BooleanField(_('Allow email'),
-                                      help_text=_('Show your email on VCS '
-                                                  'contributions.'),


I'm very 👎 on python's standard indentation style for continuation lines. It makes for very unscannable code.

def foo(): if_this_is_a_very_long_function_member_name(then_arguments_end_up_way_over_here=True, and_that_isnt_very_readable=['especially', 'when', 'we' 'start', 'cramming', 'things', 'this', 'way]) def bar(): this_is_much_more_readable( sure_its_taller=True, but=[ 'diffs make more sense', 'the end of indentation is clear', 'i can rename things without cascading indentation changes', ] )

This is very configurable in yapf. Let me know how you would like to format this and I will find the proper settings to make it fit.

If you want to get the bar formatting, you just need to add a trailing comma to the last ] and yapf will do the rest for you ;)

Considering your example, after running yapf with the settings that I pushed here:

--- yapfexample.py (original) +++ yapfexample.py (reformatted) @@ -1,15 +1,14 @@ def foo(): - if_this_is_a_very_long_function_member_name(then_arguments_end_up_way_over_here=True, - and_that_isnt_very_readable=['especially', 'when', 'we' - 'start', 'cramming', - 'things', 'this', 'way']) + if_this_is_a_very_long_function_member_name( + then_arguments_end_up_way_over_here=True, + and_that_isnt_very_readable=['especially', 'when', 'we' + 'start', 'cramming', 'things', 'this', 'way']) + def bar(): this_is_much_more_readable( - sure_its_taller=True, - but=[ + sure_its_taller=True, but=[ 'diffs make more sense', 'the end of indentation is clear', 'i can rename things without cascading indentation changes', - ] - ) + ])

Adding the trailing comma to the bar function as I mentioned:

--- yapfexample.py (original) +++ yapfexample.py (reformatted) @@ -1,8 +1,9 @@ def foo(): - if_this_is_a_very_long_function_member_name(then_arguments_end_up_way_over_here=True, - and_that_isnt_very_readable=['especially', 'when', 'we' - 'start', 'cramming', - 'things', 'this', 'way']) + if_this_is_a_very_long_function_member_name( + then_arguments_end_up_way_over_here=True, + and_that_isnt_very_readable=['especially', 'when', 'we' + 'start', 'cramming', 'things', 'this', 'way']) + def bar(): this_is_much_more_readable(

It's exactly as you want ;)

If I add two trailing commas to the fist example:

--- yapfexample.py (original) +++ yapfexample.py (reformatted) @@ -1,8 +1,18 @@ def foo(): - if_this_is_a_very_long_function_member_name(then_arguments_end_up_way_over_here=True, - and_that_isnt_very_readable=['especially', 'when', 'we' - 'start', 'cramming', - 'things', 'this', 'way',],) + if_this_is_a_very_long_function_member_name( + then_arguments_end_up_way_over_here=True, + and_that_isnt_very_readable=[ + 'especially', + 'when', + 'we' + 'start', + 'cramming', + 'things', + 'this', + 'way', + ], + ) +

or just with one:

--- yapfexample.py (original) +++ yapfexample.py (reformatted) @@ -1,8 +1,10 @@ def foo(): - if_this_is_a_very_long_function_member_name(then_arguments_end_up_way_over_here=True, - and_that_isnt_very_readable=['especially', 'when', 'we' - 'start', 'cramming', - 'things', 'this', 'way'],) + if_this_is_a_very_long_function_member_name( + then_arguments_end_up_way_over_here=True, + and_that_isnt_very_readable=['especially', 'when', 'we' + 'start', 'cramming', 'things', 'this', 'way'], + ) + def bar(): this_is_much_more_readable(

As you can see, trailing comma plays a very important role. Basically, it means: "I want this splitted element by element each in one different line"

Neat! It's nice that it yapf can be customized on a case by case basis like that. If I were to refactor the functions I wrote, I'd refactor exactly as the double trailing comma example does.

Yeah. That's the idea...

Then, the review is just something like "I would add a trailing comma there so it's easier to read" ;)

(we will have tons of those, I know why I'm telling you :) )

I just showed a couple of examples but I'm sure that there will be more complicated cases. The thing is that changing the settings to make it fit with our style wishes is simple and after changing that setting the refactor is automatic.

So, we can start with a close enough configuration and then we can continue tweaking as we discover that some cases are not covered as we want.

agjohnson · 2017-11-15T04:52:44Z

readthedocs/core/models.py

+        _('See paid advertising'),
+        help_text=_('If unchecked, you will still see community ads.'),
+        default=True,
+    )


For instance, I would refactor the above block to this

In this diff block, I would have refactored the lines that were removed with the lines that were added here.

Oh, so you like what yapf did... Not sure that I'm following you :P

agjohnson · 2017-11-15T04:54:34Z

readthedocs/core/models.py

+    )
+    allow_email = models.BooleanField(
+        _('Allow email'), help_text=_('Show your email on VCS '
+                                      'contributions.'), default=True)


And I would avoid this mixed version.

help_text gets broken up needlessly

default is almost hidden from scanning

I'm just ranting now 🙂

It got broken because the string was already splitted. I think if I remove the ' and make the string just one thing it will be more intelligent.

Yes, it will be:

allow_email = models.BooleanField( _('Allow email'), help_text=_('Show your email on VCS contributions.'), default=True)

and, if we add a trailing comma after default=True, yapf will format it like this:

allow_email = models.BooleanField( _('Allow email'), help_text=_('Show your email on VCS contributions.'), default=True, )

agjohnson · 2017-11-15T04:56:15Z

readthedocs/projects/views/base.py

 import logging
+from builtins import object


builtins is not stdlib and should be in the block below.

Import order should be:

futures

stdlib

third party

this package

relative files

don't you think it's a good idea to separate django as a section also, since it's kind of big?

I mean, instead of mixing all the django imports with the third party section

agjohnson · 2017-11-15T05:01:49Z

flake8rc

@@ -0,0 +1,3 @@
+[flake8]
+ignore = E125,D100,D101,D102,D200,P101,FI15,FI16,FI12,FI11,FI17,FI50,FI53,FI54
+max-line-length = 120


This can be moved to setup.cfg? I'd like overlap between prospector and this here if possible.

Yes, http://flake8.pycqa.org/en/latest/user/configuration.html#configuration-locations

humitos · 2017-11-16T01:08:57Z

What would workflow be? The developer would still be responsible for running this before pushing?

Mmm... I don't have a right answer for this. I have all of this commands hooked into my Emacs configuration and they are ran when I save the file. So, the workflow for other people is something that we need to discuss.

It doesn't seem like we can necessarily rely on this tooling to lint for us, but I haven't tried running these tools yet.

Why we can't rely on this? These tools modify the files in place with --in-place option (I forgot to add that to the command that I wrote). So, there is no WARNING or ERROR messages. They just modify what they need.

We'd definitely need a wrapper script as we won't remember to run that block of commands before each PR.

Yes. As I said, I have them hooked into Emacs. I wrote them to communicate to you what how I ran those commands.

agjohnson · 2017-11-16T02:26:41Z

Mmm... I don't have a right answer for this. I have all of this commands hooked into my Emacs configuration and they are ran when I save the file. So, the workflow for other people is something that we need to discuss.

For instance, I use vim and I believe @ericholscher uses sublime, so there might not be a good way to do this without requiring all of us to either run a command pre-commit or work out a way to emulate your on-save feature.

Why we can't rely on this?

I should have said solely rely on this. It will still be up to individual developers to run the autolinting before each commit -- or run periodically perhaps.

humitos · 2017-11-16T02:34:50Z

For instance, I use vim and I believe @ericholscher uses sublime, so there might not be a good way to do this without requiring all of us to either run a command pre-commit or work out a way to emulate your on-save feature

I'm not really sure how the git pre-commit command would work. I like my on-save feature because I can see how it gets formatted and I can tweak a little in case that I "don't agree" with what yapf has done... For example, add more trailing commas :)

But we can definetly figure out something that fit for everybody, even contributors.

My other co-workers used PyCharm and they wrote some minor plugins for this purpose.

humitos · 2017-11-17T22:57:18Z

yapf 0.20 released! 💛

https://github.com/google/yapf/blob/master/CHANGELOG#L5

I will need to check the new knobs and update this PR

agjohnson · 2017-11-17T23:38:05Z

In our meeting, we decided:

Let's start playing with this
Use incrementally to start
We need to clean up all PRs before doing a code-wide PR

@humitos the one piece I'd like to see to get this merged is a script (maybe in contrib/) to run all of the commands at once.

I'm treating contrib/ as a spot for things that haven't yet been enforced on development, but still useful.

humitos · 2017-11-18T02:21:21Z

@humitos the one piece I'd like to see to get this merged is a script (maybe in contrib/) to run all of the commands at once.

I will do this, but in the future I think we could consider pre-commit (I found it reading about prospector): http://pre-commit.com/#supported-languages

The steps a new contributor should follow are:

clone the repo
run pre-commit install
modify code
commit

and he/she will see a very nice message with the test that passed and the ones that not

I think we can go with a simple bash script that execute our commands over the files that where modified by the developer as a first approach since to use this pre-commit tool we will need to write a couple of wrappers. autopep8 is a good example, since the commands we need are really similar to that case: https://github.com/pre-commit/pre-commit-hooks/blob/master/pre_commit_hooks/autopep8_wrapper.py (maybe some people from the community could help us on creating this wrappers).

Besides, we will need to propose a PR on those repos with the .pre-commit-hooks.yaml so it's recognized as a plugin by pre-commit. I did it for unify as example, it's pretty simple: https://github.com/humitos/unify/blob/master/.pre-commit-hooks.yaml

I know this is more work, but after seeing that output I think it's very nice for newcomers and easy to understand what's going wrong. It will be a mixture between, automatic tools ran and fixing some issues (for example fix-encoding-pragma failed but fixed the file at the same time, so you need to add that new change to the stage now for the commit) and manual interaction, in this case flake8 that raises some "line too long".

Edit: there are some of the commands we want already done:

yapf: https://github.com/pre-commit/mirrors-yapf
isort: https://github.com/FalconSocial/pre-commit-python-sorter
https://github.com/pre-commit/pre-commit-hooks
- autopep8
- debug-statements
- double-quote-string-fixer (similar to unify)
- end-of-file-fixer
- fix-encoding-pragma
- flake8
- trailing-whitespace

So, I think the only one that it's missing is docformatter

humitos · 2017-11-18T03:01:13Z

This one could be considered in the future, also: https://github.com/asottile/pyupgrade

humitos · 2017-11-18T03:01:57Z

So, I think the only one that it's missing is docformatter

Ha, I created mine. It was pretty simple using their own tool: https://github.com/pre-commit/pre-commit-mirror-maker

Here is the repo: https://github.com/humitos/mirrors-docformatter

Use the `pre-commit` tool to run some linting commands before each commit: http://pre-commit.com/

humitos · 2017-11-18T03:14:31Z

requirements/auto-lint.txt

+flake8-quotes
+flake8-string-format
+flake8-tidy-imports
+flake8-todo


This requirements.txt file should be removed and use the additional_dependencies option for the pre-commit config files to install the "extra dependencies" for each of the plugins.

humitos · 2017-11-18T03:18:48Z

If you want to make a test over a specific file and see how pre-commit will behave, you can do:

pre-commit run --files manage.py

humitos · 2017-11-18T17:45:32Z

I was suggested another approach in Twitter by @juancarlospaco: connecting pre-commit to prospector (https://prospector.landscape.io/en/master/) via (https://github.com/guykisel/prospector-mirror#prospector-mirror) and make prospector running all of this commands instead of pre-commit plugins.

I'm not familiarized with prospector so I don't see clearly the benefits, but I @agjohnson mentioned something like this in the meeting so I suppose he will be interested on taking a look at this.

I had to add a couple of "trailing commas" to keep the format as we want

Remove the auto-lint.txt requirements file since it's not needed anymore

D105: Missing docstring in magic method

agjohnson · 2017-11-21T22:24:17Z

@humitos and I discussed this yesterday. I think we are close to merge. We'll tweak rules as we go. None of this is mandatory yet, but we'll be probably using some of these rules to autolint to strictness = high in prospector. We also discussed using prospector through precommit, which could be an option once our strictness is higher.

agjohnson · 2017-11-21T22:25:34Z

Oh, also, @humitos. Can we either revert the changes causing lint errors or fix the rules used? Error is:

pep257: D203 / 1 blank line required before class docstring (found 0)

D203 / 1 blank line required before class docstring

humitos · 2017-11-23T00:29:20Z

I just fixed the rules to fix the D203 automatically by changing a setting in yapf ;)

Everything should be working now and prospector should pass.

humitos · 2017-11-23T00:42:02Z

I think we are good here and we should merge this.

In the daily basis workflow pre-commit will modify and fix automatically (if it's possible) all the files that you want to commit. After that, you will have to check the modifications (in case you want to change something) and put them in the stage and finally commit them and push.

Then prospector will check inside the CI that the files passes all the prospector validations.

(at this moment I didn't find a way to share the config files from the commands used by pre-commit and the tools used by prospector but I think it's not a big deal by now to maintain two config files for this)

In case that pre-commit modifies files in a way that makes prospector to fail; we will need to tweak the configs that I'm pushing now.

agjohnson · 2017-11-27T16:42:06Z

Agreed! I'm looking forward to starting to use this workflow. Thanks @humitos for the tips! 🎉

ericholscher · 2017-12-03T22:27:46Z

What solved D203? I'm still getting it after my runs.

humitos · 2017-12-04T14:13:38Z

D203 is a blank line before the class docstring as here:

0f09a6f#diff-b8a9e4df8b14a1a4a0af561dcae3c19e

What kind of error are you receiving?

Config files for auto linting

8a7da01

autoflake, autopep8, docformatter, isort, yapf

humitos added the Needed: design decision A core team decision is required label Nov 15, 2017

humitos requested a review from agjohnson November 15, 2017 03:51

humitos added 3 commits November 14, 2017 22:58

readthedocs.projects.views.base with auto styling applied

70cb5d2

readthedocs.core.models auto styling applied

c4d0319

readthedocs.builds.urls with auto styling applied

2682bbc

agjohnson reviewed Nov 15, 2017

View reviewed changes

humitos added 2 commits November 15, 2017 20:33

Do not make multi line summaries

abb911d

Improve isort settings

3d79ec9

Use 80 columns as maximum

758ebb8

humitos added 2 commits November 17, 2017 22:03

pre-commit config file

979a491

Use the `pre-commit` tool to run some linting commands before each commit: http://pre-commit.com/

Update yapf config file to 0.20.0

fc1fd47

humitos commented Nov 18, 2017

View reviewed changes

humitos added 6 commits November 18, 2017 19:53

Add i18n functions to yapf config

5bc4509

Remove django as a special section for isort

606e55f

Rename config to be automatically discovered

214ea68

Add annoying as a third party lib for isort

3265354

Use the new config for all the files modified as a test

77dd236

I had to add a couple of "trailing commas" to keep the format as we want

Install additional dependencies for each of the hooks

d416783

Remove the auto-lint.txt requirements file since it's not needed anymore

humitos added 6 commits November 18, 2017 20:25

Use default config name for autopep8

978de9c

Include autoflake as pre-commit hook

80e4e85

Exclude D105 from flake8

7c0702b

D105: Missing docstring in magic method

Minor fixes from flake8

ee60db8

autoflake with --in-place

48c0253

Arguments for docformatter

e17dc14

humitos added the PR: ready for review label Nov 20, 2017

humitos added 2 commits November 22, 2017 19:27

Autofix D203 with yapf

0f09a6f

D203 / 1 blank line required before class docstring

Ignore local_settings.py on prospector

8066090

agjohnson merged commit d18ddc9 into master Nov 27, 2017

agjohnson deleted the humitos/style/autolinting branch November 27, 2017 16:42

humitos mentioned this pull request Nov 28, 2017

Add prospector as a pre-commit hook #3328

Merged

Config files for auto linting #3264

Config files for auto linting #3264

Conversation

humitos commented Nov 15, 2017 • edited Loading

humitos commented Nov 15, 2017

agjohnson left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

humitos commented Nov 16, 2017

agjohnson commented Nov 16, 2017

humitos commented Nov 16, 2017 • edited Loading

humitos commented Nov 17, 2017

agjohnson commented Nov 17, 2017

humitos commented Nov 18, 2017 • edited Loading

humitos commented Nov 18, 2017

humitos commented Nov 18, 2017

Choose a reason for hiding this comment

humitos commented Nov 18, 2017

humitos commented Nov 18, 2017

agjohnson commented Nov 21, 2017

agjohnson commented Nov 21, 2017

humitos commented Nov 23, 2017

humitos commented Nov 23, 2017

agjohnson commented Nov 27, 2017

ericholscher commented Dec 3, 2017

humitos commented Dec 4, 2017

humitos commented Nov 15, 2017 •

edited

Loading

agjohnson left a comment •

edited

Loading

humitos commented Nov 16, 2017 •

edited

Loading

humitos commented Nov 18, 2017 •

edited

Loading