xtensa-build-zephyr.py: small de-duplication #5476
Conversation
|
sof-ci/jenkins is down for the week-end |
aborisovich
left a comment
aborisovich
left a comment
There was a problem hiding this comment.
Good refactor in general, only some remarks related to the comments wording and that exception catching.
scripts/xtensa-build-zephyr.py
Outdated
There was a problem hiding this comment.
I think this is bad.
First of all, catching a BaseException is a work-around to silence pylint. It does not fix what the warning actually means in the warning message. Second, what is much more dangerous - I believe that in this operation Win32 exception may occur that is not BaseException and even not Exception derived and I think general "except" statement handles it but nothing else. Correct me if I am wrong but I gave it at lot of thought to this line writing this.
Can we just silence pylint in this case and add a comment about Win32 exception?
Adding a symlink in Windows is quite non-standard operation in my opinion.
I found in this thread that you can just do except Exception: # pylint: disable=broad-except .
I do not know what system API Python uses but some references to WinAPI symbolic link programming considerations makes me somewhat uneasy. Maybe I am overreacting. If somebody is willing to verify this on Windows please do and correct me, otherwise I would leave that "all catch" as is.
There was a problem hiding this comment.
First of all, catching a BaseException is a work-around to silence pylint.
I didn't even run pylint, I just spotted this code first without pylint and then I googled the pylint warning. This sort of catch-all is bad in any language, so I knew pylint would have a warning for this and a better "commit message" than me. Every linter will flag this in any language.
Correct me if I am wrong but I gave it at lot of thought to this line writing this.
I wasn't aware of these complexities. Silencing a warning makes sense only when there is comment that explains why + makes sure all your thoughts don't get lost!
I found in this thread that you can just do except Exception: # pylint: disable=broad-except .
I'm not familiar enough with Windows to test this. I will drop this commit from this PR. Could you submit this when you get a chance?
PS: this script has 261 pylint warnings right now.
Your code has been rated at -1.26/10
There was a problem hiding this comment.
Yeah I will check asap that exception.
PS: this script has 261 pylint warnings right now.
Your code has been rated at -1.26/10
Well, it's not surprising given that this code was rewritten from bash script and not designed from scratch the "pythonic" way :/
There was a problem hiding this comment.
Well, it's not surprising given that this code was rewritten from bash script and not designed from scratch the "pythonic" way :/
The vast majority of the pylint warnings are about indentation or mix of tabs and spaces. The second most frequent are about naming style.
There was a problem hiding this comment.
Then looks like I need to improve my python styles, Installed pylint just now so expect improvements hoho!
There was a problem hiding this comment.
Then looks like I need to improve my python styles, Installed pylint just now so expect improvements hoho!
:-)
I think your coding style is fine, some of pylint choices are subjective, especially wrt. whitespace. I think there's a way to override some of them, maybe with a header. Please go easy on the whitespace, I have work in progress and others might have too! So I would prefer a header.
A second git_submodules_update() immediately after the first one does not make sense. This looks like something leftover from a past experiment. Fixes initial commit 1de3ef3 ("Rewritten xtensa-build-zephyr.sh to python") Signed-off-by: Marc Herbert <marc.herbert@intel.com>
De-duplicate this code: if args.platforms: build_platforms() show_installed_files() Signed-off-by: Marc Herbert <marc.herbert@intel.com>
marc-hb
force-pushed
the
dedup-zephyr-py
branch
from
cb7f4d0 to
2fdacfb
Compare
marc-hb
dismissed
aborisovich’s stale review
Comments reworded, please review again
3 participants
3 commits2 commits