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

fixes #154 -- adds isort #155

Merged
merged 1 commit into from
Jan 10, 2019
Merged

fixes #154 -- adds isort #155

merged 1 commit into from
Jan 10, 2019

Conversation

sphrak
Copy link
Collaborator

@sphrak sphrak commented Jan 8, 2019

Hello,
Adds isort to testenv and sorts all imports. Closes #154 🐈

We might want to add some extra rules, personally I run these in my projects:

[isort]
combine_as_imports = true
default_section = THIRDPARTY
include_trailing_comma = true
known_first_party = django
line_length = 79
multi_line_output = 5
not_skip = __init__.py

Also, this does not test the imports in tests/. Let me know if that feels like something you might want.

Cheers mates

@sphrak sphrak changed the title adds isort fixes #154 -- adds isort Jan 8, 2019
Copy link
Collaborator

@johnraz johnraz left a comment

Choose a reason for hiding this comment

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

Cool addition 👍

And a quick question.

knox/auth.py Outdated
from knox.crypto import hash_token
from knox.models import AuthToken
from knox.settings import CONSTANTS, knox_settings
from knox.signals import token_expired
from rest_framework import exceptions
from rest_framework.authentication import (BaseAuthentication,
get_authorization_header)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we configure isort so that it gives:

from rest_framework.authentication import (	
    BaseAuthentication,	
    get_authorization_header,
)

Copy link
Collaborator Author

@sphrak sphrak Jan 9, 2019

Choose a reason for hiding this comment

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

@johnraz
Yes my friend, consider it done :)

However, do you want it exactly like you said or if you check here "5 - Hanging Grid Grouped" or "4 - Hanging Grid"? Currently the project is configure with number 5 option. Please let me know what you prefer, I think 5 is a bit more effective use of space though.

@belugame
Copy link
Collaborator

belugame commented Jan 9, 2019

awesome, 2 quick remarks:

  1. lets do it also in tests, there is no reason not to
  2. I would prefer to have knox imports below those libaries knox uses. To show which builds on what and making it the last instance to rule over others. So a knox import should be after a restframework import in my opinion.

@sphrak
Copy link
Collaborator Author

sphrak commented Jan 9, 2019

@belugame we can configure knox to be known first party as shown now in this current commit. Is that what you are looking for?

@belugame
Copy link
Collaborator

yes, great :) 👍

@belugame belugame merged commit 1e2adba into jazzband:develop Jan 10, 2019
dontexit pushed a commit to dontexit/django-rest-knox that referenced this pull request Jan 24, 2024
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