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

Removing wrong-import-position Pylint ignore statements. #1255

Merged

Conversation

dhermes
Copy link
Contributor

@dhermes dhermes commented Dec 1, 2015

Able to address by reducing the complexity of the except blocks to make Pylint happy.

@tseaver This was a follow up to #1248, which I realized was possible after filing some issues with the Pylint folks.

@dhermes dhermes added the hygiene label Dec 1, 2015
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Dec 1, 2015
@tseaver
Copy link
Contributor

tseaver commented Dec 1, 2015

Moving the imports away from the fallback implementations doesn't seem to be a win: let's just disable the check globally.

@dhermes
Copy link
Contributor Author

dhermes commented Dec 1, 2015

It doesn't seem to be a loss either. I had never questioned it before, but moving complexity out of try/except ImportError blocks isn't a bad goal to have.

By turning it off we miss out on the goodness of having stdlib > third party > local imports

@tseaver
Copy link
Contributor

tseaver commented Dec 1, 2015

I don't mind ordering the imports that way, but don't believe the dogma that "code must not be in the import section," when it forces us to move the knowledge of the alternative elsewhere. Turning off the checker seems a higher quality result than making the changes with which this PR propitiates it.

@dhermes
Copy link
Contributor Author

dhermes commented Dec 1, 2015

I don't mind ordering the imports that way

That puts the onus on us to remember, rather than letting us rely on a machine.


I think calling it dogma is a bit dramatic. We have tools that help us. Sometimes we take the bad (here: slightly inconvenient) with the good. If the alternative is writing pylint, then this is a fine trade-off.

FWIW, I actually like the changes that this hard to understand lint failure encouraged.

@tseaver
Copy link
Contributor

tseaver commented Dec 4, 2015

I'm arguing that the relatively low aesthetic value of nicely-sorted imports does not outweigh the damage of moving the fallback implementations far away from the imports they are about: this is a case where the new opinion of the pylint maintainers isn't worth the cost.

@dhermes
Copy link
Contributor Author

dhermes commented Dec 4, 2015

OK. I'll put it in pylintrc_default. Can you check out #1256 so we can discuss some of the new pylint features?

IIRC @jonparrott used a separate lint check for import order?

Moving the ignore into pylintrc_default.
@dhermes dhermes force-pushed the pylint-1.5-remove-pylint-import-issue branch from 9602add to 9a37bad Compare December 4, 2015 19:08
@dhermes
Copy link
Contributor Author

dhermes commented Dec 4, 2015

@tseaver PTAL

@tseaver
Copy link
Contributor

tseaver commented Dec 4, 2015

LGTM

dhermes added a commit that referenced this pull request Dec 4, 2015
…-issue

Removing wrong-import-position Pylint ignore statements.
@dhermes dhermes merged commit 9c95d66 into googleapis:master Dec 4, 2015
@dhermes dhermes deleted the pylint-1.5-remove-pylint-import-issue branch December 4, 2015 19:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants