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

fs: make FSStatWatcher.start private by lholmquist · Pull Request #29971 · nodejs/node · GitHub

/ node Public

fs: make FSStatWatcher.start private - #29971

Closed
lholmquist wants to merge 4 commits into
nodejs:masterfrom
lholmquist:fs-staatwatch-private
ConversationCommits (4)ChecksFiles changed
Closed

fs: make FSStatWatcher.start private#29971
lholmquist wants to merge 4 commits into
nodejs:masterfrom
lholmquist:fs-staatwatch-private

Conversation

lholmquist commented Oct 14, 2019

Copy link
Copy Markdown
Contributor

Similar to #29905

An instance of FSStatWatcher is returned when a user calls fs.watchFile,
which will call the start method. A user can't create an instance
of a FSStatWatcher directly. If the start method is called by a user
it is a noop since the watcher has already started.

ATM, a user could call start on a closed watcher and i believe it would restart it, however, they would need to re-pass the filename and other function parameters. I haven't actually tested that out, though. This is what i see from the code. It would be nice if it acted in a similar fashion to the FSWatch class, and a noop would happen if start was called on a closed watcher, but if this function becomes private, then that wouldn't matter

This method is also undocumented, in fact, the whole "Class" FSStatWatcher seems to be undocumented, although that can be a different issue/PR

This PR makes this method private, and would be a semver-major change, similar to the link issue above

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Oct 14, 2019

jasnell commented Oct 15, 2019

Copy link
Copy Markdown
Member

Unfortunately I think this will require a deprecation cycle before it can be made private

jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Oct 15, 2019

lholmquist commented Oct 15, 2019

Copy link
Copy Markdown
Contributor Author

@jasnell i'm good with whatever, but what is the difference between this PR and the previous PR i did, #29905, where there was no deprecation cycle? Both of these methods happen to be undocumented currently

jasnell commented Oct 15, 2019

Copy link
Copy Markdown
Member

That should have included a deprecation also. I hadn't seen it before it landed

lholmquist commented Oct 16, 2019

Copy link
Copy Markdown
Contributor Author

I see that new deprecation PR, #29989, for the FSWatcher.start. I'll do the same then for this one

addaleax mentioned this pull request Oct 16, 2019
Closed
3 tasks

lholmquist commented Oct 17, 2019

Copy link
Copy Markdown
Contributor Author

There seems to be some discussion happening in this issue, #29989 (comment), regarding the needing a deprecation or not, so going to wait to see what happens there before proceeding

lholmquist commented Oct 28, 2019

Copy link
Copy Markdown
Contributor Author

It looks like the outcome of #29989 (comment) , was that it is prefered to have an alias or stub than a runtime deprecation for this type of thing.

I'm going to update this PR then to do that

lholmquist commented Oct 28, 2019

Copy link
Copy Markdown
Contributor Author

Updated this PR with the stubbed out start method

lholmquist requested a review from devnexen October 28, 2019 16:18
lholmquist force-pushed the fs-staatwatch-private branch from 38b9f56 to c2ec954 Compare October 28, 2019 18:15

lholmquist commented Oct 28, 2019

Copy link
Copy Markdown
Contributor Author

Also gave this a rebase since it was out of date

lholmquist commented Oct 29, 2019

Copy link
Copy Markdown
Contributor Author

also adding in a comment, which was suggested here: #30160 (review)

I also realize i have a 2 different PR's open for the same file, sorry about that :)

fs: make FSStatWatcher.start private
7f4c116 
An instance of FSStatWatcher is returned when a user calls fs.watchFile,
which will call the start method. A user can't create an instance
of a FSStatWatcher directly. If the start method is called by a user
it is a noop since the watcher has already started.

This "Class" is currently undocumented
squash: adding back in the start method, but as a stub
0aef197
squash: add a comment for future maintainers
1ec46c6
squash: slight spelling and grammer update
7e03961
lholmquist force-pushed the fs-staatwatch-private branch from 442c5be to 7e03961 Compare November 6, 2019 15:26

lholmquist commented Nov 6, 2019

Copy link
Copy Markdown
Contributor Author

Just did a rebase now that #30160 has been merged

lholmquist commented Nov 15, 2019

Copy link
Copy Markdown
Contributor Author

@jasnell @addaleax If you have time i would appreciate your feedback.

addaleax approved these changes Nov 18, 2019
View reviewed changes

addaleax left a comment

Copy link
Copy Markdown
Member

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

Thank you for the ping!

addaleax commented Nov 18, 2019

Copy link
Copy Markdown
Member

This needs another @nodejs/tsc approval in order to land

nodejs-github-bot commented Nov 18, 2019

Copy link
Copy Markdown
Collaborator

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

addaleax added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Nov 18, 2019

Trott commented Nov 18, 2019

Copy link
Copy Markdown
Member

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2092/

addaleax pushed a commit that referenced this pull request Nov 19, 2019
fs: make FSStatWatcher.start private
535e957 
An instance of FSStatWatcher is returned when a user calls fs.watchFile,
which will call the start method. A user can't create an instance
of a FSStatWatcher directly. If the start method is called by a user
it is a noop since the watcher has already started.

This "Class" is currently undocumented.

PR-URL: #29971
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>

addaleax commented Nov 19, 2019

Copy link
Copy Markdown
Member

Landed in 535e957 🎉

addaleax closed this Nov 19, 2019
BethGriggs mentioned this pull request Mar 10, 2020
Merged
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

addaleax addaleax approved these changes
+2 more reviewers
targos targos approved these changes
devnexen devnexen approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. fs Issues and PRs related to the fs subsystem / file system. semver-major PRs that contain breaking changes and should be released in the next major 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