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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 2 commits into from May 15, 2018
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
daa0f9b
Corrected iteritems py2/3 compatability in test_api.py
cmonr May 11, 2018
99dd4c2
Added missing six requirement
cmonr May 11, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions requirements.txt
View file
Open in desktop
Expand Up @@ -14,3 +14,4 @@ fuzzywuzzy>=0.11
pyelftools>=0.24
jsonschema>=2.6
future>=0.16.0
six>=1.11.0
5 changes: 3 additions & 2 deletions tools/test_api.py
View file
Open in desktop
Expand Up @@ -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 •
edited

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.


import os
import re
Expand Down Expand Up @@ -2126,12 +2127,12 @@ def predicate(base_pred, name_base_group_case):

# Apply common directories
for pred, path in commons:
for test_identity, test_paths in tests.iteritems():
for test_identity, test_paths in six.iteritems(tests):
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.


def print_tests(tests, format="list", sort=True):
"""Given a dictionary of tests (as returned from "find_tests"), print them
Expand Down