shell_(parse|split|escape): fix some issues with deprecation #19985
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
Sponsor
Member
StefanKarpinski
commented
Jan 11, 2017
StefanKarpinski
commented
- shell_parse needed a special keyword arg but didn't have one
- seems better to have shell_split and shell_escape default to not having any special shell characters
- instead the usages of shell_parse in the implementation of the backticks syntax supplies the now-special shell characters and the corresponding use of shell_escape when displaying them too
* shell_parse needed a special keyword arg but didn't have one * seems better to have shell_split and shell_escape default to _not_ having any special shell characters * instead the usages of shell_parse in the implementation of the backticks syntax supplies the now-special shell characters and the corresponding use of shell_escape when displaying them too
|
See #19786. cc: @amitmurthy, @tkelman. This addresses your concerns from that PR by making |
tkelman
reviewed
Jan 11, 2017
|
|
||
| The unexported `shell_escape` function is the inverse of the unexported `shell_split` function: | ||
| it takes a string or command object and escapes any special characters in such a way that calling | ||
| `shell_split` on it would give back the array of words in the original command. The `special` | ||
| keyword argument controls what characters in addition to whitespace, backslashes, quotes and | ||
| dollar signs are considered to be special. Examples: | ||
| dollar signs are considered to be special (default: none). Examples: |
There was a problem hiding this comment.
"in addition" isn't accurate any more then?
There was a problem hiding this comment.
It is: whitespace, backslashes, quotes and dollar signs are always considered special; the special keyword allows you to supply a string of characters that are also considered special.
tkelman
reviewed
Jan 11, 2017
| @@ -4,7 +4,15 @@ | |||
|
|
|||
| const shell_special = "#{}()[]<>|&*?~;" | |||
|
|
|||
| function shell_parse(str::AbstractString, interpolate::Bool=true) | |||
| # needs to be factored out so depwarn only warns once | |||
| @noinline warn_shell_special(special) = | |||
There was a problem hiding this comment.
editor playing tricks on me?
felixrehren
added a commit
to felixrehren/julia
that referenced
this pull request
Feb 17, 2017
felixrehren
added a commit
to felixrehren/julia
that referenced
this pull request
Feb 17, 2017
tkelman
reviewed
May 2, 2017
| dollar signs are considered to be special (default: none). Examples: | ||
|
|
||
| julia> Base.shell_escape("cat", "/foo/bar baz", "&&", "echo", "done") | ||
| "cat '/foo/bar baz' && echo done" | ||
|
|
||
| julia> Base.shell_escape("echo", "this", "&&", "that") | ||
| "echo this '&&' that" |
There was a problem hiding this comment.
no longer displays with quotes around the &&, and these should be doctests so this would have been noticed sooner
There was a problem hiding this comment.
Please make a PR to fix it.
Merged
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.