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

Split the shebang using strings.Fields #2

Merged
merged 2 commits into from
Oct 18, 2021

Conversation

lsnow99
Copy link
Contributor

@lsnow99 lsnow99 commented Oct 18, 2021

No description provided.

@jstrieb
Copy link
Owner

jstrieb commented Oct 18, 2021

Are you sure this works with quoted arguments?

@lsnow99
Copy link
Contributor Author

lsnow99 commented Oct 18, 2021

No but I didn't think the original code did either, but I see now why you have it passing the entire string as the second argument. However I don't think that works unless you make the first argument a shell like sh or bash. The reason I have this PR is because the current code does not work on a shebang like #!deno run --allow-read=./store.json. It throws error: Found argument 'run --allow-read=./store.json' which wasn't expected, or isn't valid in this context. Do I dare suggest using an external package? ;)

@jstrieb
Copy link
Owner

jstrieb commented Oct 18, 2021

This is a valid point. I went down a bit of a rabbit hole looking at this when I wrote it the way I did, and if I was to include an external package, it would be that one – the code is simple and clear.

I was being a little obsessive about accuracy at the cost of practicality, and in the meantime, I think your PR is a solid compromise. Might just be worth amending it with a comment linking to this discussion and removing the outdated comment (since it no longer behaves as described by the comment).

Fuck it. If we're gonna fix this, might as well fix it right. It's worth adding the external package.

Once you make that change, I'll merge it. Thanks!

@jstrieb jstrieb merged commit 52f476d into jstrieb:master Oct 18, 2021
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.

2 participants