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

Options defaults #25

Merged
merged 3 commits into from
May 30, 2017
Merged

Options defaults #25

merged 3 commits into from
May 30, 2017

Conversation

rgbkrk
Copy link
Member

@rgbkrk rgbkrk commented May 8, 2017

Spawn options are now folded into a set of default options, using Object.assign
(not a full merge).

Note: this is a breaking change, particularly for Hydrogen which was providing
"loose" stdout and stderr as notification messages in Atom (I love having this
when I'm working with Spark in Hydrogen).

Fixes #24


Check out commit ce657d3, as the first commit is primarily there to bring us inline with the rest of our code bases (using prettier).

rgbkrk added 2 commits May 8, 2017 09:12
Spawn options are now folded into a set of default options, using `Object.assign`
(not a full merge).

Note: this is a breaking change, particularly for Hydrogen which was providing
"loose" stdout and stderr as notification messages in Atom (I love having this
when I'm working with Spark in Hydrogen).

Fixes #24
@williamstein
Copy link

+1

Note: this is a breaking change, particularly for Hydrogen which was providing "loose" stdout and stderr as notification messages in Atom (I love having this when I'm working with Spark in Hydrogen).

Yes, that would be really nice to do as well of course. The thing to not do is nothing at all, which results in blocked stdout...

@williamstein
Copy link

Note: this is a breaking change

Is it a breaking change for me, who is using:

require('spawnteract').launch(@name, {detached: true, stdio:'ignore'})

@rgbkrk
Copy link
Member Author

rgbkrk commented May 8, 2017

Note: this is a breaking change, particularly for Hydrogen which was providing "loose" stdout and stderr as notification messages in Atom (I love having this when I'm working with Spark in Hydrogen).

Yes, that would be really nice to do as well of course. The thing to not do is nothing at all, which results in blocked stdout...

If you want to only block stdin from the kernel process (yet not stdin on the channels):

stdio: ['ignore', 'pipe', 'pipe']

Then stderr and stdout will be piped through. That's likely what we'll switch it to for Hydrogen.

@rgbkrk
Copy link
Member Author

rgbkrk commented May 8, 2017

Is it a breaking change for me, who is using:

require('spawnteract').launch(@name, {detached: true, stdio:'ignore'})

No, your options get folded in since you're overriding the defaults here.

> defaultSpawnOptions = { stdio: "ignore" }
Object {stdio: "ignore"}
> Object.assign({}, defaultSpawnOptions, { detached: true, stdio:'ignore' })
Object {stdio: "ignore", detached: true}

@rgbkrk rgbkrk force-pushed the options-defaults branch 3 times, most recently from 8d80ebb to c745d78 Compare May 30, 2017 16:50
@rgbkrk
Copy link
Member Author

rgbkrk commented May 30, 2017

Annnnnd now this passes. I kept changing the .travis.yml until I could get the python2 kernel installed.

Copy link
Member

@lgeiger lgeiger left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Thanks for fixing CI 💚

@lgeiger lgeiger merged commit e0961d3 into master May 30, 2017
@lgeiger lgeiger deleted the options-defaults branch May 30, 2017 23:27
@rgbkrk
Copy link
Member Author

rgbkrk commented May 30, 2017

Cool, I'll ship this.

@rgbkrk
Copy link
Member Author

rgbkrk commented May 30, 2017

Unless there's something else people want to get in, we'll have to call this a major release since it changes the defaults (if user's weren't setting it -- Hydrogen for example).

@rgbkrk
Copy link
Member Author

rgbkrk commented May 30, 2017

v3.0.0 is out!

This pull request was closed.
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.

3 participants