dash-generate-components crashes on windows #915
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…nd some of the offending command line arguments. This stops dash-generate-components crashing. The quotes placed around the ignore attribute need to be removed on entry to extract-meta.js These faults have been reported here: plotly/dash-component-boilerplate#74 #913
stevej2608
requested review from
alexcjohnson,
byronz and
Marc-Andre-Rivet
as code owners
alexcjohnson
reviewed
Sep 9, 2019
| @@ -58,8 +58,11 @@ def generate_components( | |||
| reserved_patterns = "|".join("^{}$".format(p) for p in reserved_words) | |||
|
|
|||
| os.environ["NODE_PATH"] = "node_modules" | |||
|
|
|||
| fmt = "node {} \"{}\" \"{}\" {}" if is_windows else "node {} {} {} {}" | |||
There was a problem hiding this comment.
Thanks @stevej2608 !
Is it necessary that this change is only made on Windows? Can't we just always quote these arguments? Obviously we (Plotly devs) don't build on Windows very often, so I worry it'll just break again if Windows has special code.
|
I agree that allways quoting is the way to go, I'm not in a position to
test on anything other than Windows so
I went with the least risky solution. If you could give the "always quoted
solution" a go on unix then I'm sure
it will also run OK on Windows.
Cheers.
…On Mon, Sep 9, 2019 at 4:38 PM alexcjohnson ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In dash/development/component_generator.py
<#915 (comment)>:
> @@ -58,8 +58,11 @@ def generate_components(
reserved_patterns = "|".join("^{}$".format(p) for p in reserved_words)
os.environ["NODE_PATH"] = "node_modules"
+
+ fmt = "node {} \"{}\" \"{}\" {}" if is_windows else "node {} {} {} {}"
Thanks @stevej2608 <https://github.com/stevej2608> !
Is it necessary that this change is *only* made on Windows? Can't we just
*always* quote these arguments? Obviously we (Plotly devs) don't build on
Windows very often, so I worry it'll just break again if Windows has
special code.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#915?email_source=notifications&email_token=ABGWJHUCE2M2NKIXVSMK4STQIZUW7A5CNFSM4IU4NDZ2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCEC6PVA#pullrequestreview-285599700>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABGWJHXMQXPHSK5IBZEJKD3QIZUW7ANCNFSM4IU4NDZQ>
.
|
|
Confirmed that we can use these quotes in all contexts - in fact they get stripped out immediately by >>> shlex.split('a "b" c')
['a', 'b', 'c']
>>> shlex.split('a "b" c', posix=False)
['a', '"b"', 'c']So @stevej2608 if you can update to always add these quotes, I will be happy to merge. |
byronz
reviewed
Sep 9, 2019
| cmd = shlex.split( | ||
| "node {} {} {} {}".format( | ||
| "node {} \"{}\" \"{}\" {}".format( |
There was a problem hiding this comment.
minor: a more pythonic way is to use a single quote to avoid the escape
e.g. 'node {} "{}"'.format()
byronz
approved these changes
Sep 9, 2019
HammadTheOne
pushed a commit
to HammadTheOne/dash
that referenced
this pull request
May 28, 2021
HammadTheOne
pushed a commit
that referenced
this pull request
Jul 23, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
dash-generate-components crashes on windows. This fix puts quotes around some of the offending command line arguments. This stops dash-generate-components crashing. The quotes placed around the
ignoreattribute need to be removed on entry toextract-meta.jsThese problems have been reported here:
Issue with executing dash-component-boilerplate on Windows
and here:
dash-generate-components ignore not working