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

Add FSWatcher.start function docs #29872

Closed
wants to merge 1 commit into from

Conversation

lholmquist
Copy link
Contributor

There was // FIXME in the fs module for documenting the undocumented FSWatcher.start method.

This PR adds docs for that function.

Although, while looking into that function, it doesn't really make sense to have it exposed to an end user. A user can't actually create an instance of a FSWatcher by themselves, they get it as a result of calling fs.watch, which will call start when a valid filename is given. If the user calls start again on an already started watcher, then it is a noop. Calling start on a closed watcher also does nothing.

I wonder if we should consider making this a "private" method. It might be more effort than it is worth though and docs might just be enough

There are 2 more "FIXME"'s in this module related to docs that i'm planning on sending PR's for, but wanted to get some input on the above paragraph, since they are very similar

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

* This documents the  method
@nodejs-github-bot nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Oct 7, 2019
@addaleax addaleax added the doc Issues and PRs related to the documentations. label Oct 7, 2019
@addaleax
Copy link
Member

addaleax commented Oct 7, 2019

A user can't actually create an instance of a FSWatcher by themselves, they get it as a result of calling fs.watch, which will call start when a valid filename is given. If the user calls start again on an already started watcher, then it is a noop. Calling start on a closed watcher also does nothing.

So it’s essentially always a no-op from the user’s perspective? I wouldn’t document it as public API then, and instead look into making it private.

@lholmquist
Copy link
Contributor Author

lholmquist commented Oct 7, 2019

So it’s essentially always a no-op from the user’s perspective? I wouldn’t document it as public API then, and instead look into making it private.

That's what it looks like, unless there is some edge case where it is needed that i can't see. Also, there is this issue, #17430, which sort of talks about how it might not be needed.

If we do make it a private function, then what would be the process here. First, add the new private function, and also doc-deprecate the current function?

@addaleax
Copy link
Member

addaleax commented Oct 7, 2019

First, add the new private function, and also doc-deprecate the current function?

People might disagree with me on this but I’d just make it private as a semver-major PR, given that it seems useless otherwise, which makes it very likely that nobody uses it.

If we do follow a deprecation cycle on this, we could start with a runtime deprecation, because it’s undocumented to begin with.

@lholmquist
Copy link
Contributor Author

People might disagree with me on this but I’d just make it private as a semver-major PR, given that it seems useless otherwise, which makes it very likely that nobody uses it.

This would probably be my preference here, but i'll wait to here more feed back. Is there someone/team that would be helpful to ping?

@addaleax
Copy link
Member

addaleax commented Oct 7, 2019

@nodejs/tsc ^^^

@cjihrig
Copy link
Contributor

cjihrig commented Oct 7, 2019

I agree with @addaleax in #29872 (comment).

@lholmquist
Copy link
Contributor Author

@addaleax @cjihrig I'll probably give this until the end of today for others to comment, and if no one objects, i'll probably create another PR, referencing this one, that changes watcher.start to watcher._start.

should there be something added to the deprecations.md file for this(as a runtime depcrecation) or just make sure to specify that it is a semver-major pr?

@lholmquist lholmquist mentioned this pull request Oct 9, 2019
4 tasks
@BridgeAR BridgeAR added semver-major PRs that contain breaking changes and should be released in the next major version. and removed semver-major PRs that contain breaking changes and should be released in the next major version. labels Oct 9, 2019
@lholmquist
Copy link
Contributor Author

closing this now since FSWatcher.start has been removed in this PR , #29905

@lholmquist lholmquist closed this Oct 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants