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

housekeeping: sort imports #761

Merged
merged 2 commits into from
Aug 22, 2017

Conversation

tony
Copy link
Contributor

@tony tony commented Aug 22, 2017

Run module and tests through isort

@codecov-io
Copy link

codecov-io commented Aug 22, 2017

Codecov Report

Merging #761 into develop will decrease coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #761      +/-   ##
===========================================
- Coverage    98.13%   98.11%   -0.02%     
===========================================
  Files           15       15              
  Lines         1123     1117       -6     
===========================================
- Hits          1102     1096       -6     
  Misses          21       21
Impacted Files Coverage Δ
django_filters/conf.py 97.22% <ø> (ø) ⬆️
django_filters/widgets.py 100% <100%> (ø) ⬆️
django_filters/filterset.py 100% <100%> (ø) ⬆️
django_filters/utils.py 98.21% <100%> (ø) ⬆️
django_filters/rest_framework/backends.py 92.85% <100%> (ø) ⬆️
django_filters/filters.py 98.48% <100%> (-0.01%) ⬇️
django_filters/rest_framework/filterset.py 100% <100%> (ø) ⬆️
django_filters/fields.py 100% <100%> (ø) ⬆️
django_filters/views.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 0112ff5...bcb627f. Read the comment docs.

@tony tony force-pushed the sort-imports branch 2 times, most recently from 743931e to 0611bae Compare August 22, 2017 19:15
@tony
Copy link
Contributor Author

tony commented Aug 22, 2017

@carltongibson rebased

Copy link
Owner

@carltongibson carltongibson left a comment

Choose a reason for hiding this comment

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

@tony I'm happy with the sorting but I don't like the indentation change.

(I'll add an inline comment for the ones I mean.

@@ -1,17 +1,15 @@
from __future__ import absolute_import
from __future__ import unicode_literals
from __future__ import absolute_import, unicode_literals
Copy link
Owner

Choose a reason for hiding this comment

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

This kind of combination is OK

from django.utils.encoding import force_str
from django.utils.translation import ugettext_lazy as _

from .utils import handle_timezone
from .widgets import RangeWidget, LookupTypeWidget, CSVWidget, BaseCSVWidget
from .widgets import BaseCSVWidget, CSVWidget, LookupTypeWidget, RangeWidget
Copy link
Owner

Choose a reason for hiding this comment

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

Again, fine.

)
from .fields import (BaseCSVField, BaseRangeField, DateRangeField,
DateTimeRangeField, IsoDateTimeField, Lookup,
LookupTypeField, RangeField, TimeRangeField)
Copy link
Owner

Choose a reason for hiding this comment

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

Even this is OK, although...

@tony
Copy link
Contributor Author

tony commented Aug 22, 2017

ModelMultipleChoiceFilter,
MultipleChoiceFilter, NumberFilter,
OrderingFilter, RangeFilter,
TimeRangeFilter, TypedMultipleChoiceFilter)
Copy link
Owner

@carltongibson carltongibson Aug 22, 2017

Choose a reason for hiding this comment

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

These I'd prefer One Per Line, with a single indent.

from django_filters.filters import (
    AllValue...,
    etc
)

MultipleChoiceFilter, NumberFilter,
NumericRangeFilter, OrderingFilter,
RangeFilter, TimeFilter, TimeRangeFilter,
TypedMultipleChoiceFilter, UUIDFilter)
Copy link
Owner

Choose a reason for hiding this comment

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

This is the one I don't like.

(One per line with a single indent by preference.)

DateRangeFilter, Filter, FilterMethod,
ModelChoiceFilter,
ModelMultipleChoiceFilter, NumberFilter,
UUIDFilter)
Copy link
Owner

Choose a reason for hiding this comment

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

Again, the change is fine but One Per Line with a single indent. 🙂

@tony
Copy link
Contributor Author

tony commented Aug 22, 2017

@carltongibson Just rebased, how do you like it now?

@carltongibson
Copy link
Owner

Yes. I like it a lot. 😄

@@ -3,3 +3,6 @@ license-file = LICENSE

[wheel]
universal=1

[isort]
multi_line_output=3
Copy link
Owner

Choose a reason for hiding this comment

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

Good isort-foo

@carltongibson carltongibson added this to the Version 1.1 milestone Aug 22, 2017
@carltongibson carltongibson merged commit b56d2c3 into carltongibson:develop Aug 22, 2017
@tony tony deleted the sort-imports branch August 22, 2017 19:28
@rpkilby
Copy link
Collaborator

rpkilby commented Aug 22, 2017

Annnnnd @carltongibson broke all half of the other PR's 😉

Give me a sec and I'll rebase them.

@carltongibson
Copy link
Owner

Yes. Of course I did. Oops.

carltongibson pushed a commit that referenced this pull request Oct 19, 2017
housekeeping: sort imports
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants