Py3 support #8181
Py3 support #8181
Conversation
| @@ -1357,4 +1357,4 @@ def merge_build_data(filename, toolchain_report, app_type): | |||
| if 'type' not in build[0]: | |||
| build[0]['type'] = app_type | |||
| build_data['builds'].insert(0, build[0]) | |||
| dump(build_data, open(filename, "wb"), indent=4, separators=(',', ': ')) | |||
| dump(build_data, open(filename, "w"), indent=4, separators=(',', ': ')) | |||
There was a problem hiding this comment.
does this close file handle after dump ?
There was a problem hiding this comment.
I don't know how python close files but normally there is no problem even when not closing the files in code
There was a problem hiding this comment.
I think more pythonic way would be like:
with open('data.json', 'w') as outfile:
json.dump(data, outfile)
I little doubt that file handle stay open after dump call - normally if you open file with open() you need to take care to close it as well..
There was a problem hiding this comment.
Echoing @jupe's comment about using with open()
There was a problem hiding this comment.
Ah, sorry. Typed too quickly. @jupe this is ok because the file handle is only opened within the context of the function call. Once the call is completed, the reference is garbage collected.
I agree that your suggestion is more pythonic and makes sure to close the file, but considering this is a fix and this is at the end of the file, I'm ok with this going in asis.
There was a problem hiding this comment.
It is true that when there is no references to file handle it will be closed automatically . But that is not the case when exceptions happens because then reference to file handle is stored to traceback object - part of stack trace and file will not be closed.. That could happen in some corner case - even unlikely. Anyway, it would be more safe to use with statement just in case..
There was a problem hiding this comment.
Interesting. Hadn't thought about what happens in the case of an exception.
I'm fine with seeing the change be made if @OPpuolitaival wants to make it. I was also under the impression that it would be good to get this in sooner rather than later, and since this seemed like a small change/improvement, I didn't want to hold up the PR.
| @@ -1357,4 +1357,4 @@ def merge_build_data(filename, toolchain_report, app_type): | |||
| if 'type' not in build[0]: | |||
| build[0]['type'] = app_type | |||
| build_data['builds'].insert(0, build[0]) | |||
| dump(build_data, open(filename, "wb"), indent=4, separators=(',', ': ')) | |||
| dump(build_data, open(filename, "w"), indent=4, separators=(',', ': ')) | |||
There was a problem hiding this comment.
Ah, sorry. Typed too quickly. @jupe this is ok because the file handle is only opened within the context of the function call. Once the call is completed, the reference is garbage collected.
I agree that your suggestion is more pythonic and makes sure to close the file, but considering this is a fix and this is at the end of the file, I'm ok with this going in asis.
|
/morph build |
Build : SUCCESSBuild number : 3112 Triggering tests/morph test |
tools/run_icetea.py
Outdated
| @@ -169,12 +169,15 @@ def icetea_tests(target, tcdir, verbose): | |||
| command = ['icetea', '--tcdir', tcdir, '--list', '--json', '--platform_filter', target] \ | |||
| + (['-v'] if verbose else []) | |||
|
|
|||
| if not os.path.exists(tcdir): | |||
There was a problem hiding this comment.
could this be before command variable definition? it would be more readable..
Exporter Build : SUCCESSBuild number : 2719 |
|
Ready for CI ? |
|
Yes, this is ready for CI |
|
/morph build |
Build : SUCCESSBuild number : 3116 Triggering tests/morph test |
Exporter Build : SUCCESSBuild number : 2725 |
Test : SUCCESSBuild number : 2909 |
Description
Small fixes for python3 support and icetea error handling
Pull request type