-
Notifications
You must be signed in to change notification settings - Fork 229
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
Backport #285 display correct inotify limit error #290
Conversation
4287a98
to
9df2018
Compare
@@ -85,7 +87,7 @@ | |||
- META: Change commit message style: commits are now prefixed by a `[topic]`. | |||
- FIX: Make sure debounced watcher terminates. [#170] | |||
- FIX: \[Linux\] Remove thread wake-up on timeout (introduced in 4.0.5 by error). [#174] | |||
- FIX: Restore compatibility with Rust before 1.30.0. [`eab75118`] | |||
- FIX: Restore compatibility with Rust before 1.30.0. [`eab75118`] |
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.
my editor doesn't seem to like this line..
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.
👍
I think it'd be better to introduce a new error case, but this approach technically does not break semver.
We're going to v5 so this is still reasonable for me as a "patch".
I'm not sure I understand what you mean. That we're going towards a stable v5 ? Because this is the backport for v4. |
I mean, we don't have to introduce a new error structure for v4 as it's passive-developed now (sorry if my wording is strange, I'm not a native speaker :/). |
src/lib.rs
Outdated
#[cfg(target_os = "linux")] | ||
if err.raw_os_error() == Some(28) { | ||
return Error::Generic(String::from("Can't watch (more) files, limit on the total number of inotify watches reached")) | ||
} |
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.
Btw, I noticed this breaks our MSRV.
error: attributes are not yet allowed on `if` expressions
--> src/lib.rs:524:9
|
524 | #[cfg(target_os = "linux")]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^
error: aborting due to previous error
So it should be:
#[cfg(target_os = "linux")] | |
if err.raw_os_error() == Some(28) { | |
return Error::Generic(String::from("Can't watch (more) files, limit on the total number of inotify watches reached")) | |
} | |
#[cfg(target_os = "linux")] | |
{ | |
if err.raw_os_error() == Some(28) { | |
return Error::Generic(String::from("Can't watch (more) files, limit on the total number of inotify watches reached")) | |
} | |
} |
Ah ok makes sense, I'm not a native one either ;) I'll fix that suggestion (and CI failure) and then merge. |
I forgot to backport #277 to v4, will do later. |
Signed-off-by: Aron Heinecke <[email protected]>
Backport #285 to v4.
This approach is API breaking in one way or another. If we add another error kind (as #285 does), we break the error-API and if we use this approach by utilizing
Error::Generic
we make it harder to catch and also will break user code that may check for this exact error-no case to display user friendly errors. I think it'd be better to introduce a new error case, but this approach technically does not break semver.