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

Report monitored directories #6

Closed
wants to merge 1 commit into from
Closed

Report monitored directories #6

wants to merge 1 commit into from

Conversation

FND
Copy link
Contributor

@FND FND commented Dec 23, 2017

this tends to avoid confusion by confirming the watcher was set up
properly (or allowing users to recognize configuration errors)

a600a28

I found myself duplicating this in tests:
https://github.com/faucet-pipeline/faucet-pipeline/blob/f09ddc9674220352b778c132d5b427409c081cd8/index.js#L53
So it seemed sensible to add it to nite-owl instead.

NB:

  • obviously this would be a minor-version change
  • note that I added "…" as line-continuation indicators - not sure whether that makes sense

this tends to avoid confusion by confirming the watcher was set up
properly (or allowing users to recognize configuration errors)
@FND FND requested a review from moonglum December 23, 2017 11:29
@@ -11,11 +11,16 @@ class TooManyFilesError extends Error {
}
};

module.exports = (rootDirs, delay = 50) => {
module.exports = (rootDirs, { delay = 50, suppressReporting }) => {
Copy link
Member

Choose a reason for hiding this comment

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

I think that makes sense, but why not reverse the option? Make it a reporting option that defaults to false – then it's also not a breaking change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I figured it's a breaking change either way because the second argument is now an options object.

I opted for suppressReporting to make it abundantly clear that this would be changing the (new) default behavior - seemed more explicit than report: false, plus defaulting to true on a boolean argument always struck me as a little weird/unexpected. But I don't care too much either way.

More generally (cf. #7), I'm not worried about breaking changes (i.e. releasing a new minor version) at this point: Updating faucet-pipeline should be a breeze, I doubt there are any other users anyway.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, let's ship it 👍

Copy link
Member

@moonglum moonglum left a comment

Choose a reason for hiding this comment

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

👍

@FND
Copy link
Contributor Author

FND commented Dec 30, 2017

Merged as b7c5b77.

@FND FND closed this Dec 30, 2017
@FND FND deleted the reporting branch December 30, 2017 13:59
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.

2 participants