replace call to shellwords with a call to system passing an array #4
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
flavorjones
approved these changes
Dec 26, 2021
There was a problem hiding this comment.
TIL that this pattern doesn't work on Windows! This PR's code is simpler and easier to read.
I have a slight preference for exec rather than system (they both work with a splat of arguments) to avoid an additional process being created, but that's not a terribly good reason and in a development environment it's unlikely to make much difference either way.
|
If it becomes important later, we can use os.windows? and use I'm still puzzled why I couldn't get |
flavorjones
added a commit
to rails/tailwindcss-rails
that referenced
this pull request
Sep 3, 2022
Note that this is a pretty heavy refactoring, where code was moved from `exe/tailwindcss` to `lib/tailwindcss/commands.rb` where we can more easily run unit tests on it. Note also that we no longer use Shellwords to build command strings, a library which does not generate correct strings on Windows platforms. Instead we consistently use arrays of command arguments, which can be passed to `exec` or `system` however we see fit. The wrapper script conditionally uses `system` on windows platforms because `exec` can't find the executable (see related issue at rubys/sprockets-esbuild#4). Finally, note that the rake tasks no longer use the `exe/tailwindcss` wrapper script, and instead use the binary executable directly. We can reverse this decision if we ever decide to support manually-installed tailwindcss somewhere on the $PATH; but because previously the rake tasks hardcoded the path/to/exe/tailwindcss, we're not introducing any new constraints by skipping the wrapper.
anderscain63
added a commit
to anderscain63/tailwindcss-rails
that referenced
this pull request
Dec 20, 2022
Note that this is a pretty heavy refactoring, where code was moved from `exe/tailwindcss` to `lib/tailwindcss/commands.rb` where we can more easily run unit tests on it. Note also that we no longer use Shellwords to build command strings, a library which does not generate correct strings on Windows platforms. Instead we consistently use arrays of command arguments, which can be passed to `exec` or `system` however we see fit. The wrapper script conditionally uses `system` on windows platforms because `exec` can't find the executable (see related issue at rubys/sprockets-esbuild#4). Finally, note that the rake tasks no longer use the `exe/tailwindcss` wrapper script, and instead use the binary executable directly. We can reverse this decision if we ever decide to support manually-installed tailwindcss somewhere on the $PATH; but because previously the rake tasks hardcoded the path/to/exe/tailwindcss, we're not introducing any new constraints by skipping the wrapper.
titan2gman
added a commit
to titan2gman/tailwindcss-rails
that referenced
this pull request
Dec 27, 2022
Note that this is a pretty heavy refactoring, where code was moved from `exe/tailwindcss` to `lib/tailwindcss/commands.rb` where we can more easily run unit tests on it. Note also that we no longer use Shellwords to build command strings, a library which does not generate correct strings on Windows platforms. Instead we consistently use arrays of command arguments, which can be passed to `exec` or `system` however we see fit. The wrapper script conditionally uses `system` on windows platforms because `exec` can't find the executable (see related issue at rubys/sprockets-esbuild#4). Finally, note that the rake tasks no longer use the `exe/tailwindcss` wrapper script, and instead use the binary executable directly. We can reverse this decision if we ever decide to support manually-installed tailwindcss somewhere on the $PATH; but because previously the rake tasks hardcoded the path/to/exe/tailwindcss, we're not introducing any new constraints by skipping the wrapper.
smartech7
pushed a commit
to smartech7/rails-tailwind-css
that referenced
this pull request
Aug 4, 2023
Note that this is a pretty heavy refactoring, where code was moved from `exe/tailwindcss` to `lib/tailwindcss/commands.rb` where we can more easily run unit tests on it. Note also that we no longer use Shellwords to build command strings, a library which does not generate correct strings on Windows platforms. Instead we consistently use arrays of command arguments, which can be passed to `exec` or `system` however we see fit. The wrapper script conditionally uses `system` on windows platforms because `exec` can't find the executable (see related issue at rubys/sprockets-esbuild#4). Finally, note that the rake tasks no longer use the `exe/tailwindcss` wrapper script, and instead use the binary executable directly. We can reverse this decision if we ever decide to support manually-installed tailwindcss somewhere on the $PATH; but because previously the rake tasks hardcoded the path/to/exe/tailwindcss, we're not introducing any new constraints by skipping the wrapper.
XLoopLion
added a commit
to XLoopLion/tailwindcss-rails
that referenced
this pull request
Jun 12, 2024
Note that this is a pretty heavy refactoring, where code was moved from `exe/tailwindcss` to `lib/tailwindcss/commands.rb` where we can more easily run unit tests on it. Note also that we no longer use Shellwords to build command strings, a library which does not generate correct strings on Windows platforms. Instead we consistently use arrays of command arguments, which can be passed to `exec` or `system` however we see fit. The wrapper script conditionally uses `system` on windows platforms because `exec` can't find the executable (see related issue at rubys/sprockets-esbuild#4). Finally, note that the rake tasks no longer use the `exe/tailwindcss` wrapper script, and instead use the binary executable directly. We can reverse this decision if we ever decide to support manually-installed tailwindcss somewhere on the $PATH; but because previously the rake tasks hardcoded the path/to/exe/tailwindcss, we're not introducing any new constraints by skipping the wrapper.
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.
Shellwords is not compatible with Windows.
While
--helpworks, other common esbuild options do not, for example:The most obvious fix, namely calling exec with an array of arguments curiously fails on windows with an error of the executable not being found.
Surprisingly, the exact same invocation using
systeminstead ofexecworks.