rusackas commented on PR #39644:
URL: https://github.com/apache/superset/pull/39644#issuecomment-4434579263
@hainenber curious what you think about the updated state of this. Seems to
me that:
1) It's non-breaking. shlex.split("node -v") produces ["node", "-v"], which
behaves identically to the shell=True form for every hardcoded command in that
file. The FileNotFoundError catch preserves the existing graceful-degradation
behavior for missing tools. Nothing regresses.
2) The code is strictly cleaner. You lose three # noqa suppression comments
(S602, S607 × 2), the obfuscated getattr(subprocess, "check_output")
anti-pattern from the early commits is gone, and the explicit argument list in
get_docker_platform() is actually easier to read than the single-string shell
form with embedded quotes.
3) shlex.split is the right primitive. Your own review flagged .split() as
broken on paths with spaces — that concern is now addressed. shlex.split
handles it correctly, so the fix doesn't introduce a latent cross-platform bug.
But you've got the blocking review, so your call :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
