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

Replace list.sort() with sorted() #52

Closed
wants to merge 1 commit into from
Closed

Replace list.sort() with sorted() #52

wants to merge 1 commit into from

Conversation

skuicloud
Copy link
Contributor

In OpenStack heatclient project, there is error running in py33 env:
https://bugs.launchpad.net/python-heatclient/+bug/1243096

For the Python 2&3 compatability, use sorted() builtin to replace
list.sort().

This change is verified in OpenStack heatclient/novaclient projects.
(tox -e py27 / tox -e py33)

In OpenStack heatclient project, there is error running in py33 env:
https://bugs.launchpad.net/python-heatclient/+bug/1243096

For the Python 2&3 compatability, use sorted() builtin to replace
list.sort().

This change is verified in OpenStack heatclient/novaclient projects.
(tox -e py27 / tox -e py33)
@jml
Copy link
Member

jml commented Oct 22, 2013

I don't think the problem is Python 3.3 compatibility. The docs are pretty clear that list.sort works.

Instead of replacing with sorted(), please just pass the key function to sort.

@rbtcollins
Copy link
Member

I think you've misinterpreted the problem -

class Foo:
... pass
...
class Bar:
... pass
...
[Foo(), Bar()].sort()
Traceback (most recent call last):
File "", line 1, in
TypeError: unorderable types: Bar() < Foo()

Thats the issue, and sorted has the problem too:

list(sorted([Foo(), Bar()]))
Traceback (most recent call last):
File "", line 1, in
TypeError: unorderable types: Bar() < Foo()

so the thing letting your tests pass is the use of key, which can be used in list.sort() as well.

  • tests = sorted(tests, key=lambda x: x[0])

However, list.sort is very efficient, so please don't change to sorted.

@rbtcollins
Copy link
Member

In fact, a further thought occurs to me. The fact the objects are being compared at all indicates you have identical sort keys in that list. As the sort keys are the test ids, this indicates you have a bad test id in your test suite, and even if it runs you'll get bad data out. So this change isn't needed - but please open a bug that we don't give a good error message about your corrupt test suite.

@skuicloud
Copy link
Contributor Author

Agree with you, new bug is raised for this:
#56

I will make some further investigation to find the root cause of failed test id.

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.

3 participants