Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Config files for auto linting #3264

Merged
merged 23 commits into from Nov 27, 2017
Merged

Config files for auto linting #3264

merged 23 commits into from Nov 27, 2017

Conversation

@humitos
Copy link
Member

@humitos humitos commented Nov 15, 2017

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
Copy link
Member Author

@humitos humitos commented Nov 15, 2017

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.

Copy link
Contributor

@agjohnson agjohnson left a comment

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.

"""URL configuration for builds app."""
"""
URL configuration for builds app.
"""
Copy link
Contributor

@agjohnson agjohnson Nov 15, 2017

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

Copy link
Member Author

@humitos humitos Nov 16, 2017

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

--make-summaries-multilines does the trick

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'),
Copy link
Contributor

@agjohnson agjohnson Nov 15, 2017

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

Copy link
Member Author

@humitos humitos Nov 16, 2017

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.

Copy link
Contributor

@agjohnson agjohnson Nov 16, 2017

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 🙂

Copy link
Member Author

@humitos humitos Nov 16, 2017

I won't ask why... :)

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

class UserProfile(models.Model):
"""
Additional information about a User.
"""
Copy link
Contributor

@agjohnson agjohnson Nov 15, 2017

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

Copy link
Member Author

@humitos humitos Nov 16, 2017

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?

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

@agjohnson agjohnson Nov 15, 2017

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',
        ]
    )

Copy link
Member Author

@humitos humitos Nov 16, 2017

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.

Copy link
Member Author

@humitos humitos Nov 16, 2017

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 ;)

Copy link
Member Author

@humitos humitos Nov 16, 2017

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 ;)

Copy link
Member Author

@humitos humitos Nov 16, 2017

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"

Copy link
Contributor

@agjohnson agjohnson Nov 16, 2017

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.

Copy link
Member Author

@humitos humitos Nov 16, 2017

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 :) )

Copy link
Member Author

@humitos humitos Nov 16, 2017

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.

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

@agjohnson agjohnson Nov 15, 2017

For instance, I would refactor the above block to this

Copy link
Member Author

@humitos humitos Nov 16, 2017

?

Copy link
Contributor

@agjohnson agjohnson Nov 16, 2017

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

Copy link
Member Author

@humitos humitos Nov 16, 2017

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

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

@agjohnson agjohnson Nov 15, 2017

And I would avoid this mixed version.

  • help_text gets broken up needlessly
  • default is almost hidden from scanning

I'm just ranting now 🙂

Copy link
Member Author

@humitos humitos Nov 16, 2017

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,
    )

import logging
from builtins import object
Copy link
Contributor

@agjohnson agjohnson Nov 15, 2017

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

Import order should be:

  • futures
  • stdlib
  • third party
  • this package
  • relative files

Copy link
Member Author

@humitos humitos Nov 19, 2017

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

Copy link
Member Author

@humitos humitos Nov 19, 2017

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

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

@agjohnson agjohnson Nov 15, 2017

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

@humitos
Copy link
Member Author

@humitos humitos commented Nov 16, 2017

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
Copy link
Contributor

@agjohnson agjohnson commented Nov 16, 2017

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
Copy link
Member Author

@humitos humitos commented Nov 16, 2017

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
Copy link
Member Author

@humitos humitos commented Nov 17, 2017

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
Copy link
Contributor

@agjohnson agjohnson commented Nov 17, 2017

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
Copy link
Member Author

@humitos humitos commented Nov 18, 2017

@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:

  1. clone the repo
  2. run pre-commit install
  3. modify code
  4. commit

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

captura de pantalla_2017-11-17_20-50-32


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:

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

@humitos
Copy link
Member Author

@humitos humitos commented Nov 18, 2017

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

@humitos
Copy link
Member Author

@humitos humitos commented Nov 18, 2017

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

humitos added 2 commits Nov 18, 2017
Use the `pre-commit` tool to run some linting commands before each
commit:

    http://pre-commit.com/
flake8-quotes
flake8-string-format
flake8-tidy-imports
flake8-todo
Copy link
Member Author

@humitos humitos Nov 18, 2017

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
Copy link
Member Author

@humitos humitos commented Nov 18, 2017

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
Copy link
Member Author

@humitos humitos commented Nov 18, 2017

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.

@agjohnson
Copy link
Contributor

@agjohnson agjohnson commented Nov 21, 2017

@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
Copy link
Contributor

@agjohnson agjohnson commented Nov 21, 2017

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)

humitos added 2 commits Nov 23, 2017
D203 / 1 blank line required before class docstring
@humitos
Copy link
Member Author

@humitos humitos commented Nov 23, 2017

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
Copy link
Member Author

@humitos humitos commented Nov 23, 2017

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
Copy link
Contributor

@agjohnson agjohnson commented Nov 27, 2017

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

@agjohnson agjohnson merged commit d18ddc9 into master Nov 27, 2017
2 checks passed
@agjohnson agjohnson deleted the humitos/style/autolinting branch Nov 27, 2017
@ericholscher
Copy link
Member

@ericholscher ericholscher commented Dec 3, 2017

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

@humitos
Copy link
Member Author

@humitos humitos commented Dec 4, 2017

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

0f09a6f#diff-b8a9e4df8b14a1a4a0af561dcae3c19e

What kind of error are you receiving?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants