Skip to content

Conversation

@tommcdo
Copy link

@tommcdo tommcdo commented May 15, 2013

@enov
Copy link
Contributor

enov commented Nov 21, 2014

This is a well written bug fix, with tests. However, existing code relying on the numeric sequential keys of the output of this function will break, if we apply this fix.

@acoulton could you kindly comment what to do in this case?

@acoulton
Copy link
Member

Tricky. Since it changes the response, I think it would have to go to 3.4 with a clear note in the upgrade guide.

It will only be if end-user code is specifically looking at the keys, I think, that it's an issue - they'll still come out in the same sequence as far as I can see? It's probably quite an edge case, but still not safe for 3.3.

@tommcdo if you disagree we'd welcome your opinion.

@tommcdo
Copy link
Author

tommcdo commented Nov 21, 2014

I think it's quite unlikely that users are currently relying on the numerical keys with the current implementation, since they're quite frankly not useful at all. However, this is not strictly a bug fix, so it's perfectly reasonable to hold off until 3.4.

@enov
Copy link
Contributor

enov commented Nov 22, 2014

However, this is not strictly a bug fix, so it's perfectly reasonable to hold off until 3.4.

@tommcdo could you please PR this to 3.4/develop branch.

@neo22s
Copy link
Member

neo22s commented Mar 21, 2016

What we do with this? to 4.0.0? Looks good to me.

@neo22s neo22s added this to the 4.0.0 milestone Mar 21, 2016
@acoulton
Copy link
Member

I suggest a contrib manually merges this PR to the 4.0 branch and adds to upgrade notes

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.

4 participants