Skip to content
This repository has been archived by the owner on Sep 23, 2024. It is now read-only.

Fix the ordering of imports #143

Closed
wants to merge 2 commits into from

Conversation

major
Copy link
Contributor

@major major commented May 21, 2018

This PR works towards the goals of #3 by ordering imports properly and checking those imports with flake8-import-order.

@major major added the enhancement New feature or request label May 21, 2018
@major major self-assigned this May 21, 2018
@coveralls
Copy link

coveralls commented May 21, 2018

Pull Request Test Coverage Report for Build 467

  • 9 of 9 (100.0%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 46.658%

Totals Coverage Status
Change from base Build 463: 0.0%
Covered Lines: 684
Relevant Lines: 1466

💛 - Coveralls

@major major force-pushed the fix-import-ordering branch from 7807d86 to 186d474 Compare May 21, 2018 20:18
Copy link
Contributor

@veruu veruu left a comment

Choose a reason for hiding this comment

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

I'm conflicted on the results of this pull request. Some of it is fine, but some of it makes the orientation around imports more confusing and also introduces new pylint error.

flake8-import-order uses by default a very.. weird, for lack of better expression, ordering. I'd say if we are to use it, import style (and possibly application local imports) need to be properly configured first. We already established PEP8 and Google style guide complience, the tool supports checking both of these style guides. We should pick one of them and use it for the checks instead of the default which seems to go against those rules, what do you think?

import time
from email.errors import HeaderParseError
Copy link
Contributor

Choose a reason for hiding this comment

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

Moving this introduces pylint error C: 26, 0: Imports from package email are not grouped (ungrouped-imports).

import StringIO
from email.mime.application import MIMEApplication
from email.mime.multipart import MIMEMultipart
from email.mime.text import MIMEText
Copy link
Contributor

Choose a reason for hiding this comment

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

PEP8 doesn't say anything about the actual order in the groupings, but lexical order is usually used because of readability and orientation reasons. This breaks the lexical order while actually not fixing anything and making the orientation around imports more confusing?

@major
Copy link
Contributor Author

major commented May 22, 2018

Let's just forget about this one for now.

@major major closed this May 22, 2018
@major major deleted the fix-import-ordering branch May 22, 2018 12:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants