-
Notifications
You must be signed in to change notification settings - Fork 44
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
[proposal] use isort to organize python imports #91
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll approve but I would like to configure isort to ignore alphabetic order when importing modules with import
command.
import math | ||
import os |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like alphabetical ordering. I prefer to import in the same sequence as we use modules/packages on code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
correios/utils.py
Outdated
from datetime import datetime | ||
from decimal import Decimal | ||
from itertools import chain | ||
from typing import Container, Iterable, Sized, Union | ||
|
||
import re | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ugh! This import went unnoticed :|
requirements-test.txt
Outdated
@@ -1,5 +1,6 @@ | |||
-r requirements.txt | |||
factory-boy==2.8.1 | |||
isort |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isort could be used for test purposes?
.travis.yml
Outdated
@@ -18,6 +18,7 @@ install: | |||
- pip install flake8 | |||
|
|||
before_script: | |||
- isort --check |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isort --check
only outputs if the file is correct/incorrect. I believe isort --check --diff
would be a better choice. This way we'll get more details on why the test failed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to define a standard and stick to it. The mypy-lang
doesn't belongs to test suite but it's in requirements-test.txt
.
I'd like to suggest rename requirements-test.txt
to requirements-ci.txt
and put: flake8
, mypy-lang
, isort
, etc inside it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I consider these lint
command as test
environment on my projects. On ci
I run make lint
...
It's a part on development and the developer should run this command locally... my opinion.
@@ -18,6 +18,7 @@ install: | |||
- pip install flake8 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO isort shouldn't be added on requirements-test.txt
file because it's not used in the project's test suite. Instead it should be installed here with other checkers such as flake8
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
bfb0c07
to
7816ac9
Compare
Codecov Report
@@ Coverage Diff @@
## master #91 +/- ##
==========================================
- Coverage 98.98% 98.98% -0.01%
==========================================
Files 11 11
Lines 1572 1570 -2
==========================================
- Hits 1556 1554 -2
Misses 16 16
Continue to review full report at Codecov.
|
@osantana can I organize these commits? 🤔 |
May I create a Squash Merge? |
Sounds good. |
To fix:
isort -rc .
To check: