Skip to content
This repository has been archived by the owner on May 4, 2018. It is now read-only.

unix, windows: removed unused status parameter #1190

Merged
merged 1 commit into from
Mar 17, 2014
Merged

unix, windows: removed unused status parameter #1190

merged 1 commit into from
Mar 17, 2014

Conversation

saghul
Copy link
Contributor

@saghul saghul commented Mar 11, 2014

async, timer, prepare, idle and check handles don't need the status parameter.

So, this closes issue #40.

I understand this will require many changes on dependant projects (mine included), but I'd go for it because it's just more correct. Also changes are pretty simple :-)

Now, since we seem to be pretty close to v0.12, we could merge this after, I'd also be ok with that.

/cc @indutny, @piscisaureus, @bnoordhuis

@Nodejs-Jenkins
Copy link

Thank you for contributing this pull request! Here are a few pointers to make sure your submission will be considered for inclusion.

The following commiters were not found in the CLA:

  • Saúl Ibarra Corretgé

You can fix all these things without opening another issue.

Please see CONTRIBUTING.md for more information

@indutny
Copy link
Contributor

indutny commented Mar 11, 2014

Let's do it after v0.12, I'd like to have some space for ABI trickery :P

@saghul
Copy link
Contributor Author

saghul commented Mar 11, 2014

On 03/11/2014 09:22 AM, Fedor Indutny wrote:

Let's do it after v0.12, I'd like to have some space for ABI trickery :P


Reply to this email directly or view it on GitHub:
#1190 (comment)

Sure, I just wanted to get it out there and get some early feedback.
Look at the branch name ;-)

Saúl Ibarra Corretgé
bettercallsaghul.com

@indutny
Copy link
Contributor

indutny commented Mar 11, 2014

Haha, ok. Looks good.

@bnoordhuis
Copy link
Contributor

FWIW, I have no qualms about landing it before v0.12 but I don't have a strong opinion either. The delta between v0.10 and v0.12 is already huge so a few more changes is just another drop in the ocean. PR LGTM BTW.

@saghul
Copy link
Contributor Author

saghul commented Mar 11, 2014

On 03/11/2014 05:15 PM, Ben Noordhuis wrote:

FWIW, I have no qualms about landing it before v0.12 but I don't have a
strong opinion either. The delta between v0.10 and v0.12 is already huge
so a few more changes is just another drop in the ocean. PR LGTM BTW.

I just wanted to be cautious and not put work in other's shoulders. Now,
I wouldn't mind landing it before 0.12 (in fact I'd love to) and making
the required changes to Node, if @tjfontaine is ok with that.

Saúl Ibarra Corretgé
bettercallsaghul.com

@saghul
Copy link
Contributor Author

saghul commented Mar 13, 2014

Here is the companion node PR: nodejs/node-v0.x-archive#7298

async, timer, prepare, idle and check handles don't need the status
parameter.
@saghul saghul merged commit db2a907 into joyent:master Mar 17, 2014
@saghul
Copy link
Contributor Author

saghul commented Mar 17, 2014

Thanks for the comments, it landed! Let there be dragons!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants