Skip to content

Conversation

amw
Copy link
Contributor

@amw amw commented Aug 6, 2019

README suggest running flake8 before contributing, but current master reports issues.

$ flake8
./example/tests/test_views.py:14:1: I001 isort found an import in the wrong position

This fixes the current issue. Long term you'd probably want to add flake8 to the test suite and reject PRs that have flake8 problems. If you don't care about flake8 issues then maybe we should remove it from README?

@codecov
Copy link

codecov bot commented Aug 6, 2019

Codecov Report

Merging #681 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #681      +/-   ##
==========================================
- Coverage   95.96%   95.96%   -0.01%     
==========================================
  Files          54       54              
  Lines        2728     2727       -1     
==========================================
- Hits         2618     2617       -1     
  Misses        110      110
Impacted Files Coverage Δ
example/api/resources/identity.py 100% <100%> (ø) ⬆️
example/tests/test_utils.py 100% <100%> (ø) ⬆️
example/tests/test_views.py 100% <100%> (ø) ⬆️
example/tests/test_filters.py 100% <100%> (ø) ⬆️

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 7c6cceb...00ac7c3. Read the comment docs.

@sliverc
Copy link
Member

sliverc commented Aug 9, 2019

This change raises an error for me locally. Could it be that you have an outdated isort?

Also see #686

What I find really odd though is why CI is green as flake8 is already enforced. When I run tox -e flake8 locally with this change it also fails. 🤔

@amw
Copy link
Contributor Author

amw commented Aug 12, 2019

You mean it raises an isort error or does the app/pytest fail?

I don't think my isort is outdated. It's version is locked by requirements file.

$ cat requirements-development.txt | grep isort
flake8-isort==2.7.0
isort==4.3.21
$ pip freeze | grep isort
flake8-isort==2.7.0
isort==4.3.21
$ pytest -q -o console_output_style=classic
.........................................................................................................................................................................................................
201 passed in 7.41 seconds

@sliverc
Copy link
Member

sliverc commented Aug 15, 2019

Odd I get the exact same error you get with the same isort version you use but with using your fix.

Most likely an oddness in isort itself. However I don't think using relative and absolute imports mixed is a good idea. So updated this PR not to use relative imports which should also fix this error.

@sliverc sliverc self-requested a review August 15, 2019 18:26
@sliverc sliverc changed the title Fix isort issue. Replace relative imports to make isort happy Aug 15, 2019
@sliverc sliverc merged commit a226b6f into django-json-api:master Aug 15, 2019
@amw amw deleted the fix-isort-issue branch January 24, 2021 20:36
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.

2 participants