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

Allow for user-provided args. Issue #5 #6

Closed
wants to merge 1 commit into from

Conversation

deanrad
Copy link

@deanrad deanrad commented Apr 11, 2016

See Issue #5

@searls
Copy link
Member

searls commented Apr 11, 2016

Will merge once we have an safe / integration test for the behavior.

@searls
Copy link
Member

searls commented Apr 11, 2016

(The appveyor status failure can be disregarded until our windows PR is done)

@deanrad
Copy link
Author

deanrad commented Apr 11, 2016

One thing remaining - documentation

@searls
Copy link
Member

searls commented Apr 12, 2016

Nice test! Be sure to remove scripts/args/user... tho or else every user will get it!!!

@deanrad
Copy link
Author

deanrad commented Apr 12, 2016

~~ There, tied up with a 🙇 ~~
Note to self. Never say a PR is "tied up"!

Verify that user-provided args are passed through if script references $@

Readme and cleanup
runScripty('args:echoer', {userArgs: ['--test', 'arg passed by user']}, function (er, code, stdio) {
assert.includes(stdio.stdout, '--test arg passed by user')
})
done(null)
Copy link
Member

Choose a reason for hiding this comment

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

This done(null) needs to be inside runScripty's callback after the assertion (or it will exit prematurely) and called with done(er) so it'll fail if anything errors

@searls
Copy link
Member

searls commented Apr 12, 2016

Just gave some line comments. Sorry for giving you such a hard time with this

@deanrad
Copy link
Author

deanrad commented Apr 12, 2016

I knew that's what you referred to on twitter ;)

@searls
Copy link
Member

searls commented Apr 12, 2016

I have also sent you slack DMs out of an abundance of caremad.

@searls
Copy link
Member

searls commented Apr 12, 2016

Alright and just to create even more work for you, the appveyor windows build is now passing, so we should get this one passing too before merge. Any new example scripts in the integration tests need to be provided in the new fixture directories for windows (with process.platform conditionals for any divergent assertions). See 757d16e for an example

@searls
Copy link
Member

searls commented Apr 15, 2016

Closing to go work in #12 (you have commit bit now if you want to push to it)

@searls searls closed this Apr 15, 2016
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