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

fix: allow plugin callbacks to be in any language #995

Merged
merged 1 commit into from Jul 29, 2021

Conversation

Stratus3D
Copy link
Member

@Stratus3D Stratus3D commented Jul 13, 2021

The original code invoked all plugin callback scripts with Bash.
While all plugin callback scripts are currently implemented in
Bash it is conceivable that another a better shell language will
emerge and become popular. I dont think mandating Bash is the
right choice anymore. I do think that for nearly all plugins
Bash is the right choice right now and for compatibility
purposes Bash should be encouraged.

@Stratus3D Stratus3D added the WIP label Jul 13, 2021
@Stratus3D Stratus3D requested a review from a team as a code owner July 13, 2021 21:28
@jthegedus
Copy link
Contributor

Looking good. Would it be worth adding bash/sh to banned list?

@Stratus3D
Copy link
Member Author

Looking good. Would it be worth adding bash/sh to banned list?

We're still using bash in one place, and of course we use it in the shebang line because asdf written in Bash, so I don't think we are quite ready for that yet. But possibly soon, assuming we can ignore the shebang lines.

@Stratus3D Stratus3D changed the title Allow callback scripts to be implemented in any language Allow callbacks to be implemented in any language Jul 14, 2021
@jthegedus jthegedus changed the title Allow callbacks to be implemented in any language fix: allow plugin callbacks to be in any language Jul 20, 2021
@Stratus3D Stratus3D force-pushed the allow-callback-scripts-in-any-lang branch from acfda3f to 48c0ecb Compare July 29, 2021 19:30
The original code invoked all plugin callback scripts with Bash.
While all plugin callback scripts are currently implemented in
Bash it is conceivable that another a better shell language will
emerge and become popular. I dont think mandating Bash is the
right choice anymore. I do think that for nearly all plugins
Bash is the right choice right now and for compatibility
purposes Bash should be encouraged.
@Stratus3D Stratus3D force-pushed the allow-callback-scripts-in-any-lang branch from 48c0ecb to c9aca49 Compare July 29, 2021 19:33
@Stratus3D
Copy link
Member Author

@jthegedus I think this PR is ready to be merged. Should I merge it now or wait until the next release?

@jthegedus
Copy link
Contributor

jthegedus commented Jul 29, 2021

I'd just merge it now. I'd consider all fix tagged changes to be mergable ASAP. I just thought you had more to do on this because of the WIP tag.

Copy link
Contributor

@jthegedus jthegedus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jthegedus jthegedus merged commit 2ad0f5e into master Jul 29, 2021
@jthegedus jthegedus deleted the allow-callback-scripts-in-any-lang branch July 29, 2021 22:44
@Stratus3D
Copy link
Member Author

@jthegedus I overlooked the WIP label. I meant to remove it. The only thing I didn't address here was unit tests, but being that I only removed code in this PR, I figured it wasn't critical. I do want to write basic unit tests for this however. I will do so in a later PR.

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.

None yet

2 participants