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

compatibility import cleanup #906

Merged
merged 5 commits into from
Jun 9, 2017
Merged

Conversation

ploxiln
Copy link
Contributor

@ploxiln ploxiln commented Feb 23, 2017

Remove try/except blocks which were for supporting old versions of python which paramiko no longer supports.

cStringIO was actually used unconditionally before the python3 port: 0b7d0cf

@coveralls
Copy link

coveralls commented Feb 23, 2017

Coverage Status

Coverage increased (+0.1%) to 74.326% when pulling aa8163c on ploxiln:import_cleanup into 421f03b on paramiko:master.

@ploxiln
Copy link
Contributor Author

ploxiln commented Feb 23, 2017

Side note: it's a bit odd that the latest version of Fabric supports python2.5, while no supported version of paramiko does. It looks like the last version of paramiko that supported python2.5 was 1.12.4 from May of 2014.

@coveralls
Copy link

coveralls commented Feb 23, 2017

Coverage Status

Coverage increased (+0.2%) to 74.367% when pulling 6a51506 on ploxiln:import_cleanup into 421f03b on paramiko:master.

@ploxiln
Copy link
Contributor Author

ploxiln commented Feb 23, 2017

added in a bit more general import cleanup

@bitprophet
Copy link
Member

bitprophet commented Feb 23, 2017

Awesome! Looks good overall. Can you rebase against 1.18 and also open a separate PR against 2.0? For the short term I don't want to leave 1.x folks completely hanging so I'm trying to backport most changes there. (But since it requires carefully cherry-picking to 2.0, so 2.0 doesn't get feature work from 1.18, I'm probably going to stop soon. Or maybe just stop updating 2.0...hrm.)

The Fabric thing is largely laziness on my part / the fact that 2.0 is looming. Paramiko got a Py3 compat overhaul in 1.13 which is when it dropped 2.5 support; Fabric 1 can still arguably only work on Python 2 so a user with Paramiko 1.12 and Fabric 1 and Python 2.5 should still be able to operate.

@ploxiln ploxiln changed the base branch from master to 1.18 February 23, 2017 20:57
@ploxiln
Copy link
Contributor Author

ploxiln commented Feb 23, 2017

rebased on 1.18
(since this is not really features nor bug fixes, wasn't sure it made sense to do so)

I think it's not worth the effort and confusing git history to up-merge from 1.18 to 2.0, you'll probably want to start doing 1.17/1.18 together and 2.0/2.1/+ together (cherry-picking back to the older "bundle" of branches). Or just cherry-pick back for each, that's not uncommon.

@coveralls
Copy link

coveralls commented Feb 23, 2017

Coverage Status

Coverage increased (+0.1%) to 74.493% when pulling 964dd6c on ploxiln:import_cleanup into cb4fc8c on paramiko:1.18.

@bitprophet
Copy link
Member

bitprophet commented Feb 23, 2017

Yes, that's exactly what I've been doing so far, ever since 1.18/2.1 came out:

  • get the code into 1.17 via rebase or cherry-pick (this is now no longer necessary, I figured having a large support window was simply not worth the extra overhead)
  • merge up into 1.18
  • get the code into 2.0 via cherry-pick
  • merge up into 2.1 and master

It's just that, it's kind of a pain in the rear ;) especially since I had such a strong habit of simply "merge up up up!" from the past.

Anyway, I'll do the needful based on this, thank you!

@bitprophet bitprophet added this to the 1.18.3 etc milestone Feb 23, 2017
bitprophet added a commit that referenced this pull request Jun 9, 2017
@bitprophet bitprophet merged commit 964dd6c into paramiko:1.18 Jun 9, 2017
bitprophet added a commit that referenced this pull request Jun 9, 2017
@ploxiln ploxiln deleted the import_cleanup branch September 13, 2017 18:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants