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

reimplement errname lookup. #38

Merged
merged 3 commits into from
May 2, 2016

Conversation

jamestalmage
Copy link
Contributor

Fixes #31

I can not for the life of me figure out how to create a test that actually triggers the affected code in index.js. I can't create a negative error code in a way that causes !err to be true.

};

// used for testing the fallback behavior.
module.exports._test = errname;
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe we should have a convention of __test__ for when you have to attach something for testing?

@jamestalmage
Copy link
Contributor Author

Updated.

@jamestalmage
Copy link
Contributor Author

Hmm. Do you think there's more on stderr or stdout that's not being printed for this error?

@jamestalmage jamestalmage force-pushed the reimplement-errname branch from d5750c5 to eb0df1a Compare May 1, 2016 02:32
@sindresorhus
Copy link
Owner

Not really sure what you're asking.

const isWin = os.platform() === 'win32';

// simulates failure to capture process.binding('uv');
function fallback(code) {
Copy link
Contributor

@kevva kevva May 1, 2016

Choose a reason for hiding this comment

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

const fallback = code => { looks nicer imo.

Copy link
Owner

Choose a reason for hiding this comment

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

@kevva Join the discussion in sindresorhus/eslint-plugin-unicorn#3 ;)

@jamestalmage jamestalmage force-pushed the reimplement-errname branch from c5ce473 to b2a3b2b Compare May 1, 2016 23:49
@jamestalmage
Copy link
Contributor Author

Not really sure what you are asking

I couldn't figure out why there was a test error. Turns out I was testing on Node 4 locally when I thought I had switched to Node 0.12.

It seems uv.errname(-2) causes a process.exit on Windows Node 0.12.

I'm not even sure how to get negative error codes in Windows (or at all really).

@jamestalmage
Copy link
Contributor Author

I think this is ready for merge. Not sure what to do about the 0.12 issue, but the JS code for the part of execFile that calls uv.errname is the same all the way back to 0.12, so I don't think it's a problem.

@sindresorhus
Copy link
Owner

Not sure what to do about the 0.12 issue

Nothing. We're doing exactly what Node.js is doing and I personally couldn't care less about 0.12.

@sindresorhus sindresorhus merged commit a1d1764 into sindresorhus:master May 2, 2016
@sindresorhus
Copy link
Owner

👍

@jamestalmage jamestalmage deleted the reimplement-errname branch May 2, 2016 06:44
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