Fixes Issues #182 - Broken config file watching. #184

Merged
merged 1 commit into from Nov 10, 2012

Conversation

Projects
None yet
3 participants
Contributor

ciaranj commented Oct 28, 2012

Implements fix as suggested by Peter Schuster in comment thread.

Signed-off-by: ciaranj ciaranj@gmail.com

lib/config.js
- fs.watch(file, function (curr, prev) {
- if (curr.ino != prev.ino) { self.updateConfig(); }
+ fs.watch(file, function (event, filename) {
+ if (event != 'rename') { self.updateConfig(); }
@mrtazz

mrtazz Nov 2, 2012

Owner

is there a reason why we test for != 'rename' and not just == 'change'? Judging from the docs only those two events are available and checking for the actual one we want might be more clear.

@ciaranj

ciaranj Nov 2, 2012

Contributor

True, I just cut'n'pasted the suggested patch to get a pull request in as this is so broken at the minute. Would you prefer it this way around ?

@peschuster

peschuster Nov 2, 2012

My thought was that 'rename' is clearly no change in content, so we can exclude it. But in case there will be more event types in the future (no idea what this could be), it won't harm to reload the config. With a condition on 'change' we would miss these. But at the moment it would not make any difference.

@mrtazz

mrtazz Nov 3, 2012

Owner

My thought was that 'rename' is clearly no change in content, so we can
exclude it. But in case there will be more event types in the future (no
idea what this could be), it won't harm to reload the config. With a
condition on 'change' we would miss these. But at the moment it would not
make any difference.
I just think that using 'change' is cleaner, since we are actually watching
the file for changes. If there are ever other events added, we can adapt to
that, but even then it might not make sense to just exclude 'rename'.

@peschuster

peschuster Nov 3, 2012

I'm totally fine with that.

Contributor

ciaranj commented Nov 4, 2012

So, just to confirm I swap to == 'change' and we're good to merge?

Owner

mrtazz commented Nov 4, 2012

yup

Contributor

ciaranj commented Nov 6, 2012

Doneski ;)

Fixes Issues #182 - Broken config file watching.
Implements fix as suggested by Peter Schuster and Daniel
Schauenberg in comment thread.

Signed-off-by: ciaranj <ciaranj@gmail.com>
Owner

mrtazz commented Nov 10, 2012

awesome! thanks for the patch.

mrtazz added a commit that referenced this pull request Nov 10, 2012

Merge pull request #184 from ciaranj/fix_config_js_watcher
Fixes Issues #182 - Broken config file watching.

@mrtazz mrtazz merged commit 5a4609f into etsy:master Nov 10, 2012

1 check passed

default The Travis build passed
Details

hbouvier pushed a commit to hbouvier/statsd that referenced this pull request May 25, 2013

Merge pull request #184 from ciaranj/fix_config_js_watcher
Fixes Issues #182 - Broken config file watching.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment