FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fs.watch error message includes filename by charlierudolph · Pull Request #2748 · nodejs/node · GitHub

/ node Public

fs.watch error message includes filename - #2748

Closed
charlierudolph wants to merge 1 commit into
nodejs:masterfrom
charlierudolph:cr-fsWatch
ConversationCommits (1)ChecksFiles changed
Closed

fs.watch error message includes filename#2748
charlierudolph wants to merge 1 commit into
nodejs:masterfrom
charlierudolph:cr-fsWatch

Conversation

charlierudolph commented Sep 9, 2015

Copy link
Copy Markdown

Brought over from nodejs/node-v0.x-archive#25542

brendanashworth added the fs Issues and PRs related to the fs subsystem / file system. label Sep 9, 2015
cjihrig reviewed Sep 9, 2015
View reviewed changes
Comment thread lib/fs.js Outdated

cjihrig Sep 9, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Can you use let instead of var, and a template string instead of concatenation.

cjihrig commented Sep 9, 2015

Copy link
Copy Markdown
Contributor

You changed two code paths, but only added a test for one. Can you test for the other too?

charlierudolph commented Sep 9, 2015

Copy link
Copy Markdown
Author

Thanks for the review. Code comments addressed. I don't actually know how to test the other code path. which is an error occurring while watching a file. Any suggestions?

thefourtheye reviewed Sep 9, 2015
View reviewed changes
Comment thread test/parallel/test-fs-watch-error.js Outdated

thefourtheye Sep 9, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This is not necessary.

charlierudolph Sep 9, 2015

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

unfortunately it is. If the function doesn't return true, it throws the error.

thefourtheye Sep 9, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@charlierudolph Oops, sorry. My bad.

sindresorhus commented Sep 9, 2015

Copy link
Copy Markdown

Even if it's non-standard, might be worth going with camelCased fileName for consistency with other envs: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error/fileName

cjihrig commented Sep 9, 2015

Copy link
Copy Markdown
Contributor

One important difference is that Error.prototype.fileName indicates the file that raised the exception, while the proposed addition in this PR indicates a parameter to a file system operation.

charlierudolph commented Sep 16, 2015

Copy link
Copy Markdown
Author

@cjihrig bump. Anything else this PR needs?

thefourtheye reviewed Sep 16, 2015
View reviewed changes
Comment thread lib/fs.js Outdated

thefourtheye Sep 16, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This can actually be const

cjihrig commented Sep 17, 2015

Copy link
Copy Markdown
Contributor

It LGTM, but I'd really like a test for the emit path. Would anyone object to a slightly artificial test like:

var watcher = fs.watch(__filename);
watcher._handle.onchange(-1, 'ENOENT', 'foo.js');

charlierudolph commented Sep 29, 2015

Copy link
Copy Markdown
Author

okay @cjihrig the emit path now has a test.

cjihrig reviewed Sep 29, 2015
View reviewed changes
Comment thread test/sequential/test-fs-watch.js Outdated

cjihrig Sep 29, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Instead of creating watcherErrorCaught, you can just wrap this function in common.mustCall(). That way you can ensure it was called, and fail the test if it's not.

cjihrig commented Sep 29, 2015

Copy link
Copy Markdown
Contributor

One comment, but this LGTM

cjihrig commented Sep 29, 2015

Copy link
Copy Markdown
Contributor

Oh, and can you squash this down to a single commit?

charlierudolph commented Sep 30, 2015

Copy link
Copy Markdown
Author

Switched to using common.mustCall and squashed to a single commit.

cjihrig commented Sep 30, 2015

Copy link
Copy Markdown
Contributor

CI - https://ci.nodejs.org/job/node-test-pull-request/404/

charlierudolph commented Sep 30, 2015

Copy link
Copy Markdown
Author

That build is very complex to traverse. Okay from the link provided I got to
https://ci.nodejs.org/job/node-test-binary-arm/88/RUN_SUBSET=5,nodes=pi1-raspbian-wheezy/tapTestReport/
which says test-stringbytes-external.js is what failed. I don't know how I could have made that fail, especially since no error message is provided.

The build page shows another commit is being tested in addition to mine.
https://ci.nodejs.org/job/node-test-binary-arm/88/

thefourtheye commented Sep 30, 2015

Copy link
Copy Markdown
Contributor

Don't worry about that test. It fails often.

cjihrig added the semver-minor PRs that contain new features and should be released in the next minor version. label Oct 1, 2015
cjihrig mentioned this pull request Oct 1, 2015
Closed
cjihrig pushed a commit that referenced this pull request Oct 2, 2015
fs: include filename in watch errors
87e820e 
This commit adds the relevant filename to fs.watch() errors.

Refs: nodejs/node-v0.x-archive#25542
PR-URL: #2748
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

cjihrig commented Oct 2, 2015

Copy link
Copy Markdown
Contributor

Thanks! Landed in 87e820e

cjihrig closed this Oct 2, 2015
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

fs Issues and PRs related to the fs subsystem / file system. semver-minor PRs that contain new features and should be released in the next minor version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL