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

Corrected iteritems py2/3 compatability in test_api.py #6886

Merged
merged 2 commits into from
May 15, 2018

Conversation

cmonr
Copy link
Contributor

@cmonr cmonr commented May 11, 2018

Description

Fixes an iteritems compatibility between py2 and py3.

Discovered whilst finalizing py3 support for mbed-cli circleci tests.

Pull request type

[x] Fix
[ ] Refactor
[ ] New target
[ ] Feature
[ ] Breaking change

@cmonr
Copy link
Contributor Author

cmonr commented May 11, 2018

/morph build

@@ -17,6 +17,7 @@
Author: Przemyslaw Wirkus <Przemyslaw.wirkus@arm.com>
"""
from __future__ import print_function
import six
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

six is not currently a required package from mbed-os, so we can't use this without adding it as a requirement.

But see my other suggestion below.

Copy link
Contributor Author

@cmonr cmonr May 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh. I was under the impression that six was already required due to #5022. It looks like requirements needs to be updated regardless.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh, yeah that's definitely using that. Its a very common package so maybe it just works most of the time? Well maybe it wouldn't hurt to add it as an explicit dependency in requirements.txt?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My guess is that a different package ends up installing it as a dependency. I noticed it showed up after a fresh virtual environment.

if pred(test_identity):
test_paths.append(path)

# Drop identity besides name
return {name: paths for (name, _, _, _), paths in tests.iteritems()}
return {name: paths for (name, _, _, _), paths in six.iteritems(tests)}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of using six and iteritems, you can just use items.

return {name: paths for (name, _, _, _), paths in tests.items()}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm... When I first came across the bug, I found something pointing to items() as performing much worse than six.iteritems(). Since I can't seen to find that document again, I'll make the change.

@mbed-ci
Copy link

mbed-ci commented May 11, 2018

Build : SUCCESS

Build number : 1981
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/6886/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

@mbed-ci
Copy link

mbed-ci commented May 11, 2018

@mbed-ci
Copy link

mbed-ci commented May 11, 2018

@cmonr
Copy link
Contributor Author

cmonr commented May 11, 2018

/morph build

@mbed-ci
Copy link

mbed-ci commented May 11, 2018

Build : SUCCESS

Build number : 1982
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/6886/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

@mbed-ci
Copy link

mbed-ci commented May 11, 2018

@mbed-ci
Copy link

mbed-ci commented May 11, 2018

@cmonr
Copy link
Contributor Author

cmonr commented May 12, 2018

/morph export-build

@mbed-ci
Copy link

mbed-ci commented May 12, 2018

@cmonr
Copy link
Contributor Author

cmonr commented May 14, 2018

@bridadan If you're alright with simply adding six to requirements, could you re-review?

@cmonr cmonr merged commit 83d7444 into ARMmbed:master May 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants