-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
shell: add more extensions and interpreters #3708
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stellar work!
samples/Shell/mksh-env
Outdated
@@ -0,0 +1,2 @@ | |||
#!/usr/bin/env mksh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to @mirabilos, this line should be used:
#!/bin/mksh
“This search” is broken, the standard shebang is
#!/bin/mksh
,
only some (predominantly older) scripts use#!/usr/bin/env mksh
.Please make sure both are recognised for scripts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Updated.
samples/Shell/ash-env
Outdated
@@ -0,0 +1,2 @@ | |||
#!/usr/bin/env ash | |||
echo "ash" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We prefer to have samples from "real" applications (under permissive license). Alternatively, you can move those to tests/fixtures/
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can move both these and the previous ones (bash
and dash
) which I used as template to tests/fixtures
. Although I'm not sure which test would be checking it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lib/linguist/languages.yml
Outdated
@@ -4110,6 +4110,8 @@ Shell: | |||
- ".command" | |||
- ".fcgi" | |||
- ".ksh" | |||
- ".mksh" | |||
- ".pdksh" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neither pdksh used to nor mksh uses the shell name as script extension, we use just .sh
or no extension in the general case, .ksh
is seen rarely and more for AT&T ksh scripts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 13 mksh https://github.com/search?utf8=%E2%9C%93&q=extension%3Amksh+mksh&type=
- 14 pdksh https://github.com/search?utf8=%E2%9C%93&q=extension%3Apdksh+pdksh&type=
Indeed, it's pretty rare. Removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the pull request!
I left comments inline for a few things I think we should address before merging.
* ash: only interpreter, extension is more commonly used for Kingdom of Loathing scripting, e.g. github.com/twistedmage/assorted-kol-scripts * dash: only interpreter, extension is more commonly used for dashboarding-related stuff * ksh: extension was already present * mksh * pdksh
@pchaigno Done. |
ash: only interpreter, extension is more commonly used for
Kingdom of Loathing scripting, e.g. github.com/twistedmage/assorted-kol-scripts
dash: only interpreter, extension is more commonly used for
dashboarding-related stuff
ksh: extension was already present
mksh
pdksh