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

Drop python 2.6 support #934

Merged
merged 1 commit into from May 7, 2015
Merged

Conversation

Tarrasch
Copy link
Contributor

@Tarrasch Tarrasch commented May 5, 2015

@Tarrasch
Copy link
Contributor Author

Tarrasch commented May 7, 2015

Should I assume no complaints as OK to merge? :)

@jcrobak @daveFNbuck

@Tarrasch
Copy link
Contributor Author

Tarrasch commented May 7, 2015

Merging for now to speed up Travis build times. At least. Anyone feel free to audit this decision.

Tarrasch added a commit that referenced this pull request May 7, 2015
@Tarrasch Tarrasch merged commit d987e1e into spotify:master May 7, 2015
@Tarrasch Tarrasch deleted the remove-26-support branch May 7, 2015 09:27
@erikbern
Copy link
Contributor

erikbern commented May 7, 2015

Is Python 2.6 support a big deal to maintain? I don't think it is

@Tarrasch
Copy link
Contributor Author

Tarrasch commented May 7, 2015

It was a big deal when Spotify was relying on it. I just wanted to give a fair chance for other big users of luigi to object.

By the way @erikbern, it's time for you to re-introduce your patch where you were about to remove OptParse support. Now there's no blockers to remove it :D

@erikbern
Copy link
Contributor

erikbern commented May 7, 2015

What I meant is what's the cost of maintaining 2.6 support? Is it very high? If so, we should drop it. If it's low, we should keep it.

It seems low to me but I could be wrong

@erikbern
Copy link
Contributor

erikbern commented May 7, 2015

The optparse issue was actually in 2.7.9 btw

@erikbern
Copy link
Contributor

erikbern commented May 7, 2015

afaik some things can be simplified if we drop 2.6, like we can use functools.total_ordering for DateInterval

@Tarrasch
Copy link
Contributor Author

Tarrasch commented May 7, 2015

Ah, I see what you mean now. I think we shouldn't claim to maintain it without anybody using it. It could lead to strange bugs that tests don't cover and affect only 2.6. As I mentioned in my mail to luigi-user@.

@erikbern
Copy link
Contributor

erikbern commented May 7, 2015

Well... we have 2.6 tests running in Travis so shouldn't that be almost enough?

But if no one uses it then sure let's just drop it. I don't care too much

@Tarrasch
Copy link
Contributor Author

Tarrasch commented May 7, 2015

Nope, evidently it's not enough: https://github.com/spotify/luigi/pull/793/files

The other major benefit is the speedup in Travis build times. We're talking about a few minutes at least it seems per PR iteration. It's really annoying to wait for all the tests to pass when developing heavily on luigi.

@erikbern
Copy link
Contributor

erikbern commented May 7, 2015

But 2.6 tests are run in parallel right?

Anyway let's drop it and see if people complain... should be easy to add back later

@daveFNbuck
Copy link
Contributor

2.6 is the main cause of test failures for my commits, because I like to
use test functions that aren't present. Glad to see it leaving.

On Thu, May 7, 2015 at 5:47 AM, Erik Bernhardsson notifications@github.com
wrote:

But 2.6 tests are run in parallel right?

Anyway let's drop it and see if people complain... should be easy to add
back alter


Reply to this email directly or view it on GitHub
#934 (comment).

@Tarrasch
Copy link
Contributor Author

Tarrasch commented Jun 8, 2015

The optparse issue was actually in 2.7.9 btw

@erikbern: Does this mean that we need to maintain optparse still? I'm not sure what you meant there. I really want to remove it!

In #612 it sounded it was just because Spotify needed it for whatever reason. I'm pretty sure that's not the case for Spotify anymore. Any reason to not repoen #612?

@erikbern
Copy link
Contributor

erikbern commented Jun 8, 2015

We can prob just remove optparse, not sure why anyone would use it

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.

None yet

3 participants