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

Update isort script to match changes in the new release, isort v5.0.2 #1670

Merged
merged 11 commits into from
Jul 9, 2020
Merged

Update isort script to match changes in the new release, isort v5.0.2 #1670

merged 11 commits into from
Jul 9, 2020

Conversation

asheux
Copy link
Contributor

@asheux asheux commented Jul 5, 2020

Apparently, isort latest release isort v5.0.2 is missing the --recursive or -rc flag and since the project uses the flag in it's script, it causes the tests to fail. An alternative to this is a dot, as in isort ..

Missing flags

--apply
--recursive

Replacement

isort .

For more information on these changes see:
https://github.com/timothycrosley/isort/blob/develop/CHANGELOG.md#500-penny---july-4-2020

With this change, the build should be okay.

@codecov
Copy link

codecov bot commented Jul 5, 2020

Codecov Report

Merging #1670 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master     #1670   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          236       236           
  Lines         7150      7152    +2     
=========================================
+ Hits          7150      7152    +2     
Impacted Files Coverage Δ
fastapi/dependencies/utils.py 100.00% <ø> (ø)
fastapi/utils.py 100.00% <ø> (ø)
fastapi/security/http.py 100.00% <100.00%> (ø)
fastapi/security/oauth2.py 100.00% <100.00%> (ø)
...rial/test_additional_responses/test_tutorial001.py 100.00% <100.00%> (ø)
...rial/test_additional_responses/test_tutorial002.py 100.00% <100.00%> (ø)
...rial/test_additional_responses/test_tutorial003.py 100.00% <100.00%> (ø)
...rial/test_additional_responses/test_tutorial004.py 100.00% <100.00%> (ø)
...l/test_additional_status_codes/test_tutorial001.py 100.00% <100.00%> (ø)
...orial/test_advanced_middleware/test_tutorial001.py 100.00% <100.00%> (ø)
... and 81 more

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 3ff504f...e2e2920. Read the comment docs.

@asheux
Copy link
Contributor Author

asheux commented Jul 5, 2020

I'm trying to get rid of this error on travis ci build but I can seem to figure out how to make the build pass. Any ideas?

ERROR: /home/travis/build/tiangolo/fastapi/fastapi/utils.py Imports are incorrectly sorted and/or formatted.

ERROR: /home/travis/build/tiangolo/fastapi/fastapi/dependencies/utils.py Imports are incorrectly sorted and/or formatted.

ERROR: /home/travis/build/tiangolo/fastapi/fastapi/openapi/models.py Imports are incorrectly sorted and/or formatted.

ERROR: /home/travis/build/tiangolo/fastapi/tests/test_tutorial/test_additional_status_codes/test_tutorial001.py Imports are incorrectly sorted and/or formatted.

ERROR: /home/travis/build/tiangolo/fastapi/tests/test_tutorial/test_security/test_tutorial006.py Imports are incorrectly sorted and/or formatted.

ERROR: /home/travis/build/tiangolo/fastapi/tests/test_tutorial/test_security/test_tutorial001.py Imports are incorrectly sorted and/or formatted.

ERROR: /home/travis/build/tiangolo/fastapi/tests/test_tutorial/test_security/test_tutorial003.py Imports are incorrectly sorted and/or formatted.

ERROR: /home/travis/build/tiangolo/fastapi/tests/test_tutorial/test_security/test_tutorial005.py Imports are incorrectly sorted and/or formatted.

ERROR: /home/travis/build/tiangolo/fastapi/tests/test_tutorial/test_wsgi/test_tutorial001.py Imports are incorrectly sorted and/or formatted.

ERROR: /home/travis/build/tiangolo/fastapi/tests/test_tutorial/test_extra_models/test_tutorial003.py Imports are incorrectly sorted and/or formatted.

ERROR: /home/travis/build/tiangolo/fastapi/tests/test_tutorial/test_extra_models/test_tutorial004.py Imports are incorrectly sorted and/or formatted.

ERROR: /home/travis/build/tiangolo/fastapi/tests/test_tutorial/test_extra_models/test_tutorial005.py Imports are incorrectly sorted and/or formatted.

ERROR: /home/travis/build/tiangolo/fastapi/tests/test_tutorial/test_additional_responses/test_tutorial001.py Imports are incorrectly sorted and/or formatted.

ERROR: /home/travis/build/tiangolo/fastapi/tests/test_tutorial/test_additional_responses/test_tutorial003.py Imports are incorrectly sorted and/or formatted.

ERROR: /home/travis/build/tiangolo/fastapi/tests/test_tutorial/test_additional_responses/test_tutorial004.py Imports are incorrectly sorted and/or formatted.

@asheux
Copy link
Contributor Author

asheux commented Jul 6, 2020

This was an easy fix. Apparently, with the new update in the command, the travis ci cache was updated so I had to run the formating script on my machine and push the changes to update travis cache.

Copy link
Member

@Kludex Kludex left a comment

Choose a reason for hiding this comment

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

Tutorial files should consider fastapi imports as third party.

@@ -1,7 +1,6 @@
"""FastAPI framework, high performance, easy to learn, fast to code, ready for production"""

__version__ = "0.58.1"

Copy link
Member

Choose a reason for hiding this comment

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

isort is removing this line? If yes, could you check why?

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'm not sure but looking into it.

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 is being fixed by isort team. see PyCQA/isort#1283

@@ -1,11 +1,10 @@
from typing import Optional

from fastapi import FastAPI
Copy link
Member

Choose a reason for hiding this comment

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

We don't want to use isort in this tutorial, because fastapi and pydantic are really third party imports here.

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 is fixed

@@ -1,6 +1,5 @@
from fastapi.testclient import TestClient
Copy link
Member

Choose a reason for hiding this comment

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

We don't want to use isort in this tutorial, because fastapi are really third party here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this supposed to be in all the tutorials? If so, I can tell isort to ignore the files.

Copy link
Member

Choose a reason for hiding this comment

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

I think all of them use fastapi as third party... If that's true, yes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's correct. I've made adjustment in the new isort configuration file. This should be flexible.

@tiangolo tiangolo merged commit fe453f8 into fastapi:master Jul 9, 2020
@tiangolo
Copy link
Member

tiangolo commented Jul 9, 2020

Thanks for your contribution @asheux ! 🍰

Thanks for the help here @Kludex

I just updated it to use the Black profile and moved the config to pyproject.toml to simplify it.

I also updated the tests for the tutorial examples so that they can detect the sources as first party, without having to disable isort for big sections of code.

Thanks for your contribution! 🚀

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.

3 participants