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

Add six imports #1675

Closed
wants to merge 2 commits into from
Closed

Add six imports #1675

wants to merge 2 commits into from

Conversation

QuLogic
Copy link
Member

@QuLogic QuLogic commented May 21, 2015

This is a boring, but rather large change.

All it is is imports, no code change (except for the new test).

@marqh
Copy link
Member

marqh commented May 22, 2015

the tests have fallen over. I suspect they will fail due to an unrelated upgrade to proj4. If this occurs, rebasing to master ought to fix this

please may you explain the benefit of importing six into every module, I don't understand why this helps?
thank you

@QuLogic
Copy link
Member Author

QuLogic commented May 22, 2015

please may you explain the benefit of importing six into every module, I don't understand why this helps?

Well, by itself, nothing. But in order to implement #1658, it will be needed in the vast majority of files. In the interest of consistency, I think it's best to include it everywhere. That way, you don't need to go changing imports when you refactor things.

@pelson
Copy link
Member

pelson commented May 26, 2015

I can see both sides of the argument for including six in every module. The two things that push me off the fence though are:

  • unused import warnings on linters
  • unable to see what has actually changed in this PR due to the high number of changes

For that reason, I'm kind of falling for the only import six where it is needed approach. @QuLogic - would that make #1658 much harder? Do you have a feeling for how many modules will need six when it comes to Py3 compatibility?

@QuLogic
Copy link
Member Author

QuLogic commented May 26, 2015

unable to see what has actually changed in this PR due to the high number of changes

Ah, that's why I split it into two commits for you. The first is solely import six, while the second is just a test for it.

Do you have a feeling for how many modules will need six when it comes to Py3 compatibility?

That being said, it's actually turning out to be a quite a few less than I expected. At this point, it's:

$ git di --shortstat py3k-six..py3k
 77 files changed, 344 insertions(+), 345 deletions(-)

@QuLogic
Copy link
Member Author

QuLogic commented Jun 4, 2015

Soo, decisions? I have more PRs that could be opened based on/off this (as you can see above).

@rhattersley
Copy link
Member

  1. In favour of the bulk import we have...

    you don't need to go changing imports when you refactor things.

  2. Arguing against we have...

    • unused import warnings on linters
    • unable to see what has actually changed in this PR due to the high number of changes

NB. Doing (1) doesn't get rid of the need for Python 3 testing to ensure compatibility - just because a module imports six doesn't mean the code uses it where necessary.

Given all that, I'm inclined towards only adding the imports when and where they're used.

@pelson ?

@ajdawson
Copy link
Member

ajdawson commented Jun 4, 2015

I'm siding with 2. These aren't like the future imports where there are implicit behaviour changes due to the import alone. I think we would be better off with six imports only where necessary.

@QuLogic
Copy link
Member Author

QuLogic commented Jun 5, 2015

I think @pelson was leaning towards 2 as well. Though if you were looking for behavioural changes, one could import, e.g., six.moves.range and six.moves.zip to ensure use of Python 3 functions. Actually, this might be a better course of action, because it makes the code Python 3 and thus easier to keep working when dropping Python 2 (a long long time from now, I'd guess).

@QuLogic
Copy link
Member Author

QuLogic commented Jun 12, 2015

I'm going to do this a smaller bit at a time. Hopefully that'll work out a bit faster.

@QuLogic QuLogic closed this Jun 12, 2015
@pelson
Copy link
Member

pelson commented Jun 12, 2015

I'm going to do this a smaller bit at a time. Hopefully that'll work out a bit faster.

Thanks @QuLogic. I'm pretty comfortable with your subsequent PR (#1699), so hopefully we can move this on. 👍

@QuLogic QuLogic deleted the py3k-six branch July 7, 2015 21:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants