X Tutup
Skip to content

JS: Shelljs improvement#14291

Merged
erik-krogh merged 2 commits intogithub:mainfrom
am0o0:amammad-js-CodeInjection_Shelljs
May 17, 2024
Merged

JS: Shelljs improvement#14291
erik-krogh merged 2 commits intogithub:mainfrom
am0o0:amammad-js-CodeInjection_Shelljs

Conversation

@am0o0
Copy link
Contributor

@am0o0 am0o0 commented Sep 22, 2023

The Shelljs package has a piping feature and I've updated the current shelljs module to support piping too.

Comment on lines +15 to +18
.getMember([
"exec", "cd", "cp", "touch", "chmod", "pushd", "find", "ls", "ln", "mkdir", "mv",
"rm", "cat", "head", "sort", "tail", "uniq", "grep", "sed", "to", "toEnd", "echo"
])
Copy link
Contributor

Choose a reason for hiding this comment

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

I took a look at the type-definition of ShellJS, and from that documentation it appears that only some of the functions behave the way you have modelled here.
https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/shelljs/index.d.ts#L830

However, looking at the implementation (and specifically this PR), it seems that the type is misleading, and that your model is correct.

@erik-krogh
Copy link
Contributor

erik-krogh commented May 16, 2024

I also did some small internal evaluations, and they showed no difference in results or performance.

@am0o0
Copy link
Contributor Author

am0o0 commented May 16, 2024

@erik-krogh can I mark this pull request as Ready for review?

@erik-krogh
Copy link
Contributor

@erik-krogh can I mark this pull request as Ready for review?

Yes.

@am0o0 am0o0 marked this pull request as ready for review May 16, 2024 12:16
@am0o0 am0o0 requested a review from a team as a code owner May 16, 2024 12:16
@erik-krogh erik-krogh merged commit 03cf9b7 into github:main May 17, 2024
@am0o0 am0o0 deleted the amammad-js-CodeInjection_Shelljs branch September 14, 2024 11:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

X Tutup