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

Don't add 'absolute_import' futures indiscriminately #165

Merged
merged 1 commit into from Apr 21, 2018

Conversation

Projects
None yet
3 participants
@MartinFalatic

MartinFalatic commented Mar 27, 2018

Currently the 'absolute_import' future is added anywhere a non-future import happens. This branch removes that side-effect. The existing 'import' fixer has a very similar effect and can be directly enabled or disabled by the user.

Fixes #163 #142

@MartinFalatic

This comment has been minimized.

MartinFalatic commented Mar 27, 2018

I'm looking at the test failures now. Some are a matter of updating the test expectations, but others involve idempotency issues that the blanket 'absolute_import' adds hid previously. I suspect the first pass adds imports (e.g., 'six') where there were none before (thus the 'import' filter doesn't trigger to add the 'absolute import' future on that pass). The second pass, now with an import, triggers the 'import' filter.

While it is good to have idempotency when possible, this is an example of where two filters will interact in a way that requires a second pass to fully realize.

Edit: It appears almost all the errors may be idempotency issues related to check_on_input().

@MartinFalatic MartinFalatic force-pushed the MartinFalatic:mff-fix-touch-import branch 2 times, most recently from e2fa10d to 033bac3 Mar 27, 2018

@MartinFalatic MartinFalatic force-pushed the MartinFalatic:mff-fix-touch-import branch from 033bac3 to 1ae2cc9 Apr 11, 2018

@takluyver

This comment has been minimized.

Member

takluyver commented Apr 12, 2018

Idempotence is a bigger concern for @daira and @brettcannon than it is for me (based on e.g. #44 and #54), so I'd like them to take a look at this. I suspect that idempotence might be why we're adding absolute_import in the first place.

@MartinFalatic

This comment has been minimized.

MartinFalatic commented Apr 12, 2018

My argument is that forcing single-pass idempotence by adding absolute_import indiscriminately as was done is much more of a problem than accepting single-run idempotence (which internally involves an extra pass here). If the fixers were truly independent (and didn't fix more than they needed to) then this wouldn't come up, but they are interdependent in modernize.

To reiterate, my fix is still idempotent, it just avoids unnecessary out-of-band code changes that cause side-effects whilst trying to side-step the fact that interdependent fixers require an extra internal pass.

@takluyver

This comment has been minimized.

Member

takluyver commented Apr 12, 2018

Thanks. Idempotence was never something terribly important to me, but it's something that I know other people spent a lot of time on, so I'd like them to weigh in before I merge any changes that affect it.

@brettcannon

This comment has been minimized.

brettcannon commented Apr 12, 2018

I don't care anymore. 😄

@MartinFalatic

This comment has been minimized.

MartinFalatic commented Apr 12, 2018

I value idempotence - the worst situation is code that never stops mutating, and the second worse is not knowing when it should stop mutating. I'm satisfied that idempotence is still maintained and that this behavior should be stable within the bounds of the tests available, as well as the useful lifetime of this tool.

@takluyver

This comment has been minimized.

Member

takluyver commented Apr 12, 2018

I think @daira was the most keen on idempotence previously, so I'll still give her a few days to comment on this. If we don't hear anything, then I think we can carry on.

@MartinFalatic

This comment has been minimized.

MartinFalatic commented Apr 20, 2018

Just checking in as it's been a week now. And note again that idempotence is being preserved here, without the previously out-of-band application of the import fixer.

When this is merged, can we get a point release please?

@takluyver

This comment has been minimized.

Member

takluyver commented Apr 21, 2018

Thanks for your patience, I'll merge this now.

@takluyver takluyver merged commit 7dcdf76 into python-modernize:master Apr 21, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@takluyver

This comment has been minimized.

Member

takluyver commented Apr 21, 2018

Released 0.6.1.

@MartinFalatic MartinFalatic deleted the MartinFalatic:mff-fix-touch-import branch Apr 23, 2018

@MartinFalatic

This comment has been minimized.

MartinFalatic commented Apr 23, 2018

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment