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

Add isort to code formatting and CI #1933

Merged
merged 7 commits into from Oct 20, 2020
Merged

Conversation

MatthewFlamm
Copy link
Contributor

Fixes #1919

I'm starting this PR with adding CI for isort, which is already installed in the lint environment and already has some configuration in the repository.

Made the line length 80 to match the black setting.

Once it is confirmed that the CI works as expected, i.e. fails, I will push the next commit with the isort changes.

@codecov
Copy link

codecov bot commented Oct 17, 2020

Codecov Report

Merging #1933 into develop will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #1933   +/-   ##
========================================
  Coverage    76.97%   76.97%           
========================================
  Files           55       55           
  Lines         4704     4704           
========================================
  Hits          3621     3621           
  Misses        1083     1083           
Impacted Files Coverage Δ
mopidy/internal/playlists.py 87.09% <100.00%> (ø)
mopidy/internal/process.py 34.61% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8954abc...023844a. Read the comment docs.

@MatthewFlamm
Copy link
Contributor Author

MatthewFlamm commented Oct 17, 2020

Is there any way to get access to the circleci failure messages without giving access to circleci to all of my repositories? I want to confirm that the failures are as expected before proceeding.

@MatthewFlamm
Copy link
Contributor Author

Also I added the I line to flake8, but it is probably redundant. Would it be better to take it out, so that an import failure only fails the isort check?

@jodal
Copy link
Member

jodal commented Oct 19, 2020

I thought CircleCI would allow anyone to browse the results, but I see that it asks me to login when clicking into the job details :-/

If you really don't want to login, here are the gist of the output from the test run:

flake8 run-test: commands[0] | python -m flake8 --show-source --statistics
./mopidy/internal/playlists.py:6:1: I100 Import statements are in the wrong order. 'import xml.etree.ElementTree' should be before 'from mopidy.internal import validation' and in a different group.
import xml.etree.ElementTree as elementtree  # noqa: N813
^
./mopidy/internal/process.py:6:1: I100 Import statements are in the wrong order. 'import _thread' should be before 'import pykka' and in a different group.
import _thread
^
./mopidy/http/actor.py:7:1: I201 Missing newline between import groups. 'import tornado.httpserver' is identified as Third Party and 'import pykka' is identified as Third Party.
import tornado.httpserver
^
./tests/test_ext.py:5:1: I201 Missing newline between import groups. 'import pytest' is identified as Third Party and 'import pkg_resources' is identified as Third Party.
import pytest
^
./tests/internal/test_http.py:4:1: I201 Missing newline between import groups. 'import requests' is identified as Third Party and 'import pytest' is identified as Third Party.
import requests
^
./tests/internal/test_http.py:5:1: I201 Missing newline between import groups. 'import responses' is identified as Third Party and 'import requests' is identified as Third Party.
import responses
^
./tests/http/test_server.py:3:1: I100 Import statements are in the wrong order. 'import tempfile' should be before 'import urllib'
import tempfile
^
./tests/http/test_server.py:4:1: I100 Import statements are in the wrong order. 'import shutil' should be before 'import tempfile'
import shutil
^
./tests/core/test_playback.py:4:1: I201 Missing newline between import groups. 'import pytest' is identified as Third Party and 'import pykka' is identified as Third Party.
import pytest
^
./tests/stream/test_playback.py:6:1: I201 Missing newline between import groups. 'import requests.exceptions' is identified as Third Party and 'import pytest' is identified as Third Party.
import requests.exceptions
^
./tests/stream/test_playback.py:7:1: I201 Missing newline between import groups. 'import responses' is identified as Third Party and 'import requests.exceptions' is identified as Third Party.
import responses
^
4     I100 Import statements are in the wrong order. 'import xml.etree.ElementTree' should be before 'from mopidy.internal import validation' and in a different group.
7     I201 Missing newline between import groups. 'import tornado.httpserver' is identified as Third Party and 'import pykka' is identified as Third Party.

Since isort got support for Black, I guess we can replace the following in pyproject.toml:

ulti_line_output = 3
include_trailing_comma = true
force_grid_wrap = 0
use_parentheses = true
line_length = 88

With just:

profile = black

@@ -12,6 +12,6 @@ multi_line_output = 3
include_trailing_comma = true
force_grid_wrap = 0
use_parentheses = true
line_length = 88
line_length = 80
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is configured to 88 because that's the max limit Black allows when it is configured to try to keep to 80.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This project has changed the default black line length, which requires this change in isort too.

line-length = 80

Let me test profile=black add above and see if it uses the black config.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see what you are saying now, but black failed when using line-length=88 in isort.

tox.ini Outdated Show resolved Hide resolved
@MatthewFlamm
Copy link
Contributor Author

MatthewFlamm commented Oct 19, 2020

Thanks for following up. These are the expected failures, so we know that it fails correctly.

@MatthewFlamm MatthewFlamm marked this pull request as ready for review October 19, 2020 17:21
@MatthewFlamm
Copy link
Contributor Author

I've tested both line_length = 88 and profile = black in the isort config, and neither allows both isort and black to agree with each other. I believe this is because this project has changed the default black line-length.

Now I'm getting errors for the flake8 check for third-party libs that I need to look into.

@MatthewFlamm
Copy link
Contributor Author

Sorry for all the pushes, but sometimes the CI behavior of isort and flake8 can differ compared to the local behavior. I wanted to make sure I was on the right path before continuing.

The final piece is that flake8-import-order is not compatible with isort, so I switched it for flake8-isort. I also added it to the docs.

Copy link
Member

@jodal jodal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great :-)

@jodal jodal merged commit 5ebce75 into mopidy:develop Oct 20, 2020
@kingosticks
Copy link
Member

Thank you!

@MatthewFlamm MatthewFlamm deleted the import_order branch October 20, 2020 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

flake8-import-order violations are being ignored
3 participants