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 script argument regression, add process.argv.lsc #1014

Merged
merged 1 commit into from Mar 23, 2018

Conversation

@rhendric
Copy link
Collaborator

@rhendric rhendric commented Mar 6, 2018

In commit 03c6e6a, removing the code that mutated process.argv also removed a side effect that prevented all arguments to lsc from being handled as scripts to compile and run. This caused a regression when running lsc script-file.ls arg1 arg2 that would result in a crash when no arg1.ls file could be found.

The regression is fixed by reintroducing the side effect to remove all but the first positional argument from the array containing the scripts to compile and run, but only when lsc is running scripts—batch compilation commands involving multiple input files should remain unaffected.

This commit also adds a process.argv.lsc property, present only when a script is being interpreted by lsc, which is an array holding the name of the script being invoked along with any extra arguments not consumed by lsc. The intent here is to provide a way to get script arguments that's simpler and more reliable than picking through process.argv and skipping over elements like node, the lsc compiler itself, and any CLI options being provided to either.


Fixes #1013. This is both a bug fix and an experimental feature (the process.argv.lsc part), so I would like to hear some feedback on whether this mild alternative to the original process.argv mutation that I nuked back in #959 is reasonable—if not, it wouldn't be hard to remove that part and just make this the bug fix.

In commit 03c6e6a, removing the code that mutated process.argv also
removed a side effect that prevented all arguments to lsc from being
handled as scripts to compile and run. This caused a regression when
running `lsc script-file.ls arg1 arg2` that would result in a crash when
no `arg1.ls` file could be found.

The regression is fixed by reintroducing the side effect to remove all
but the first positional argument from the array containing the scripts
to compile and run, but only when lsc is running scripts--batch
compilation commands involving multiple input files should remain
unaffected.

This commit also adds a `process.argv.lsc` property, present only when a
script is being interpreted by lsc, which is an array holding the name
of the script being invoked along with any extra arguments not consumed
by lsc. The intent here is to provide a way to get script arguments
that's simpler and more reliable than picking through process.argv and
skipping over elements like node, the lsc compiler itself, and any CLI
options being provided to either.
@rhendric rhendric changed the title fix script argument regression fix script argument regression, add process.argv.lsc Mar 6, 2018
@rhendric
Copy link
Collaborator Author

@rhendric rhendric commented Mar 16, 2018

Crickets and thumbs up, so I'm issuing final call for comments on this. Will merge on or after Friday, March 23 if nobody has anything to say.

@rhendric rhendric added this to In progress in Release 1.6.0 Mar 17, 2018
@rhendric rhendric merged commit be5c030 into gkz:master Mar 23, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Release 1.6.0 automation moved this from In progress to Done Mar 23, 2018
@corwin-of-amber
Copy link

@corwin-of-amber corwin-of-amber commented Mar 23, 2019

Passing process.argv.lsc to command line argument parsers (e.g. commander) results in an error since these in fact expect the node executable to be the first argument. So I have to do something like:

opts.parse (if process.argv.lsc then ['', ...process.argv.lsc] else process.argv)
@rhendric
Copy link
Collaborator Author

@rhendric rhendric commented Mar 25, 2019

@corwin-of-amber, that's an interesting point. Part of why I wanted to add the .lsc property instead of continuing to mutate process.argv itself was that I didn't think there was a good way to uniformly adhere to the various expectations that different libraries placed on process.argv; this feels like another one of those. But maybe there's little harm in adding a dummy entry to the start of process.argv.lsc just so that it can easily be used as a substitute in this particular use case. It strikes me as a hack, but maybe it's worth it?

@corwin-of-amber
Copy link

@corwin-of-amber corwin-of-amber commented Mar 25, 2019

Sounds to me like process.argv.lsc[0] should be lsc itself – as it is the interpreter running the current script (similar to bash, python, node etc.).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Release 1.6.0
  
Done
Linked issues

Successfully merging this pull request may close these issues.

2 participants