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

Updates tools to be runnable in Python 3 #6592

Merged
merged 2 commits into from Apr 17, 2018

Conversation

Projects
None yet
4 participants
@cmonr
Contributor

cmonr commented Apr 10, 2018

Description

Modifies tools to complete Python 3 porting.

Tested using the following command within two Python virtual environments (Python 2.7.14 and 3.6.4): reset && find . -name *pyc -delete && clear && PYTHONPATH=. coverage run -a -m pytest tools/test

Pull request type

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

@@ -393,7 +393,7 @@ def format_validation_error(self, error, path):
return self.format_validation_error(error.context[0], path)
else:
return "in {} element {}: {}".format(
path, str(".".join(str(p) for p in error.absolute_path)), error.message)
path, str(".".join(str(p) for p in error.absolute_path)), error.message).replace('u\'', '\'')

This comment has been minimized.

@theotherjimmy

theotherjimmy Apr 10, 2018

Contributor

I'd like to see this done another way, possibly without the outer str.

This comment has been minimized.

@theotherjimmy

theotherjimmy Apr 10, 2018

Contributor
diff --git a/tools/config/__init__.py b/tools/config/__init__.py
index 04677215e..cbfc980af 100644
--- a/tools/config/__init__.py
+++ b/tools/config/__init__.py
@@ -393,7 +393,7 @@ class Config(object):
             return self.format_validation_error(error.context[0], path)
         else:
             return "in {} element {}: {}".format(
-                path, str(".".join(str(p) for p in error.absolute_path)), error.message)
+                path, ".".join(str(p) for p in error.absolute_path), error.message)
 
     def __init__(self, tgt, top_level_dirs=None, app_config=None):
         """Construct a mbed configuration

This comment has been minimized.

@cmonr

cmonr Apr 10, 2018

Contributor

I feel like I'm missing something. Implementing the above diff does not appear to work across Py versions.

In Py2, the error returned looks like:
[<Validation Error: "Additional properties are not allowed (u'unknown_key' was unexpected)">]

In Py3:
[<Validation Error: "Additional properties are not allowed ('unknown_key' was unexpected)">]

This comment has been minimized.

@theotherjimmy

theotherjimmy Apr 10, 2018

Contributor

Oh dang it. That's a string that's coming directly from jsonschema. We may have to allow both versions...

This comment has been minimized.

@cmonr

cmonr Apr 10, 2018

Contributor

Also, for reference, error.message is typed as a string in this function, which from what I understand, the u' is already a part of the variable.

This comment has been minimized.

@cmonr

cmonr Apr 10, 2018

Contributor

Oh dang it. That's a string that's coming directly from jsonschema.

Yeah. It does it's own internal conversion to support both Py versions.

We may have to allow both versions...

Are you referring to having to support both within json parsing, or just for the particular tests?

This comment has been minimized.

@theotherjimmy

theotherjimmy Apr 10, 2018

Contributor

Alright, can we move the .replace to the error.message then? I don't want to accidentally replace more than we want to.

This comment has been minimized.

@theotherjimmy

theotherjimmy Apr 10, 2018

Contributor

EH, not that important.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Apr 10, 2018

/morph build

Modified error formatting to remove unicode character.
jsonschema's error.message returns either u' or ' in strings depending on the python version.

@cmonr cmonr force-pushed the cmonr:py3-tools-port branch from 155b3f4 to 85ce95d Apr 10, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Apr 10, 2018

Restarting CI since commit came after build command.

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Apr 10, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci

This comment has been minimized.

@cmonr cmonr changed the title from Modifies tools to run in Python 3 to Updates tools to be runnable in Python 3 Apr 11, 2018

@cmonr cmonr referenced this pull request Apr 11, 2018

Closed

python 2->3 #609

@mbed-ci

This comment has been minimized.

@cmonr cmonr added ready for merge and removed needs: CI labels Apr 17, 2018

@cmonr cmonr merged commit 934101e into ARMmbed:master Apr 17, 2018

12 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 9563 cycles (+870 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/littlefs Passed, code size is 10092B (+0.00%)
Details
travis-ci/tools Local tools testing has passed
Details

@cmonr cmonr removed the ready for merge label Apr 17, 2018

@cmonr cmonr referenced this pull request May 10, 2018

Merged

Added python3 compatability #667

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment