-
Notifications
You must be signed in to change notification settings - Fork 248
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
issue #509: raise Listen::Error::INotifyMaxWatchesExceeded rather than abort #545
issue #509: raise Listen::Error::INotifyMaxWatchesExceeded rather than abort #545
Conversation
12c3273
to
5ae7b48
Compare
5ae7b48
to
a678b9e
Compare
BTW, I'm expecting to release this in v3.7.0 in the coming week. |
a678b9e
to
2972907
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks okay to me. However, I'm not sure if the exception message needs to be so elaborate?
2972907
to
62ae13d
Compare
@ioquatix I carried the exception across from the old warning message. I like that it points to the documentation.
|
62ae13d
to
254dff9
Compare
254dff9
to
9c7a4a7
Compare
I think the exception message shouldn't contain documentation. So, I'd just say "Max watches exceeded!". Personally, I don't mind either approach, but if you start adding URLs to exception messages, surely that should apply to all exceptions? So, it's hard to know where to draw the line. Perhaps having documentation on the exception class which explains what to do to mitigate it. |
I don't see any downside to the exception including a link to documentation on how to resolve the problem! The reason we don't do it all the time is that it's a lot of work. ;) In this case, someone before me already did it, and I confirmed that the link still works (since I refactored it into the README a couple months ago) so I think we're good. I will trim it down to the more succinct version, though. |
Fix for issue #509:
INotifyMaxWatchesExceeded
rather than abort.