automated import sorting and better pull request checks

[Tim Graham]

As a code reviewer, I've really appreciated that we've cleaned up the code using flake8 and have automated "enforcement" via the CI server. I'd like to extend the idea to apply to standardizing import ordering using isort [1]. This mail is to advise everyone of the change and to address any concerns.

Documentation:
https://github.com/django/django/pull/4008

Automated cleanups:
https://github.com/django/django/pull/4009

On a related note, the UI for flake8 errors isn't currently very good (you have to look at the bottom of the Jenkins build logs). I'd like to improve that by using a separate service that uses the GitHub status API [2] so that the check can run separately from the tests build. I don't want to reinvent the wheel though, so if there is an existing service we can use for this, great. I'd like to also add checks for documentation build and spelling errors, as well as isort checks (assuming adoption per above).

[1] https://github.com/timothycrosley/isort#readme
[2] https://github.com/blog/1935-see-results-from-all-pull-request-status-checks

[Marc Tamlyn]

Would it be possible to use travis for flake8/docs checks? That would hopefully automatically show up as two separate flags on the status API. Of course it would be really nice if the docs and flake8/isort and tests all displayed separately.

An alternative could be to use code climate which currently has as private alpha for python: https://codeclimate.com/python

Marc

--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
To post to this group, send email to django-d...@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/d356e388-fa45-4fdf-871f-ce484a7b43bf%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Carl Crowder]

Hi Tim / all,

I've been running a "linting as a service" for a while now called Landscape - https://landscape.io . It runs pep8, pyflakes and mccabe as well as pylint and some other things (such as pyroma and pep257 optionally). You then get a report of the errors found as well as diffs since the last commit. It might be what you're looking for.

It will create comments on PRs but does not yet use the GitHub status API; this is because the GitHub OAuth scopes required to do so are a bit generous and put off some people. I'm trying to figure out the best way to do that, as I want to keep it as read-only as possible.

I'm also happy to add features and work to make it work better for Django, feedback is always good.

Any questions, fire away!

Carl


PS: Some example pages:
* Project overview: https://landscape.io/github/landscapeio/prospector
* Changes for a particular commit (or PR): https://landscape.io/github/spotify/luigi/79/diff
* Comment on PRs: https://github.com/mozilla/treeherder-service/pull/343#issuecomment-71704436

[Aymeric Augustin]

Hi Tim,

Good idea. There are so many changes on PR 4009 that I couldn't check if you wrote a .isort.cfg. Did you, or are you just using the defaults?

-- 

Aymeric.

--

You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
To post to this group, send email to django-d...@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/d356e388-fa45-4fdf-871f-ce484a7b43bf%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

--

Aymeric.

[Tim Graham]

Here's the current iteration of the config (it's stored in setup.cfg).

[isort]
combine_as_imports = true
known_first_party = django
multi_line_output = 5
skip = build,.git

[Collin Anderson]

I personally prefer multi_line_output = 4 (closing parentheses on the same line), but no big deal either way.

[Zach Borboa]

Hi Tim,

I've always found pylint a pleasure to work with. It may be worth looking into if we want additional automated testing.

Pylint has great set of code checks (unused variable, unused import, trailing whitespace, bad indentation, etc.) and can integrate with Jenkins.

https://bitbucket.org/logilab/pylint/src/default/checkers/variables.py#cl-191

https://bitbucket.org/logilab/pylint/src/default/checkers/format.py#cl-65

Thanks,

Zach

[aRkadeFR]

I support the idea of introducing isort.

Didn't know isort before and give it a try on personal project.
It considers only the global import of the file (I mean on the
very top on my files and not in the methods / functions). Which
is exactly what it should do from my point of view.

[Andrew Ingram]

For what it's worth, I prefer one import per line, if only because it makes diffs that involve changes to imports a lot easier to digest, and usually smaller.

ie, something like this: https://gist.github.com/AndrewIngram/16da35699bd495f79adb

Andy

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/54CB5092.90507%40arkade.info.

[Tim Graham]

Two new builders run on all pull requests:

• ​flake8 - runs the equivalent of flake8 from the root checkout directory and fails if there are any warnings
• ​docs - runs the equivalent of make spelling from the docs directory and fails if there are any spelling or sphinx errors.

The spelling builder accidentally triggered on all open PRs so that's why a lot of them appear as "pending."

I've submitted a couple patches to isort and am awaiting a new release there before continuing with that cleanup in Django.

[Tim Graham]

isort has released a new version, so I've finished this task by merging the cleanups in Django and adding another pull request builder to verify isort doesn't complain on new changes. Documentation is available here: https://docs.djangoproject.com/en/dev/internals/contributing/writing-code/coding-style/#imports

Hopefully this allows contributors to more easily handle these fixes on their own in advance of a review from a human!