Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make Base.shell_escape() and Base.shell_split() public #53510

Merged
merged 2 commits into from
May 1, 2024

Conversation

JamesWrigley
Copy link
Contributor

Base.shell_escape() is particularly useful since it's the only way (AFAICT) to properly convert a Cmd to a string.

@JamesWrigley
Copy link
Contributor Author

(bump)

@StefanKarpinski
Copy link
Sponsor Member

I'm definitely on board with making them public. Should we also export them?

@StefanKarpinski StefanKarpinski added the triage This should be discussed on a triage call label Mar 29, 2024
@JamesWrigley
Copy link
Contributor Author

Hmm to me they're not quite reached-for-often-enough to be worth exporting, but I don't feel very strongly about it.

@StefanKarpinski
Copy link
Sponsor Member

Yeah, that's fair. Will discuss next triage call, but that shouldn't block merging this as is.

@oscardssmith
Copy link
Member

triage says public is good.

@oscardssmith oscardssmith removed the triage This should be discussed on a triage call label Apr 11, 2024
@Seelengrab
Copy link
Contributor

Can this be done for the other platform specific shell escapes as well?

@JamesWrigley
Copy link
Contributor Author

Do you mean these?

  • Base.shell_escape_posixly()
  • Base.shell_escape_csh()
  • Base.shell_escape_wincmd()
  • Base.escape_microsoft_c_args() (slightly different in that it takes a list of arguments and returns a single string)

@Seelengrab
Copy link
Contributor

Seelengrab commented Apr 11, 2024

Yes, exactly. Their docstrings are very in-depth, and I was planning on putting them in the manual in some form so that the docstrings of Cmd/run can reference them, to provide an anchor point for people that e.g. have to call cmd.exe directly (which uses special parsing of arguments, so should be aware of the tradeoffs).

Base.shell_escape() is particularly useful since it's the only way to properly
convert a Cmd to a string.
Note that Base.escape_raw_string() is already in the manual so it should be
public anyway.
@JamesWrigley
Copy link
Contributor Author

Sure, I guess that makes sense. Made them public in 5d89485, along with Base.escape_raw_string() because it's referenced by one of the other docstrings (and is actually already in the manual so it should be public anyway).

@JamesWrigley
Copy link
Contributor Author

(bump)

@oscardssmith oscardssmith merged commit e25ce08 into JuliaLang:master May 1, 2024
7 checks passed
@JamesWrigley JamesWrigley deleted the public-shell-functions branch May 1, 2024 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants