-
Notifications
You must be signed in to change notification settings - Fork 256
feat: rework PathsMut to be more consistent #705
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
Open
riberk
wants to merge
9
commits into
notify-rs:main
Choose a base branch
from
riberk:paths-mut-rework
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
7b6f59c
feat: rework PathsMut to be more consistent
riberk 791a04a
fix: remove paths mut
riberk 765696e
fix: deprecate paths_mut, introduce WatchPathConfig and some renames
riberk 433ecda
chore: update changelog for #705
riberk 1a03e80
feat: UpdatePathsError into Error conversion
riberk 01d0fdd
fix: typos
riberk fa43ee0
fix(test): macos fsevents may return non-canonicalized paths
riberk 98c5c0b
Apply suggestions from code review
riberk 22a9efa
remove paths_mut due to an major release
riberk File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
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 added
committo give future implementations a chance to return errors at the time of commit. It was an intentional design choice with its own downside. Now on second thought, inconsistent states are indeed more harmful.How about just replacing
commitwithdrop?Would this change solve the inconsistent state?
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 would, but it seems a bit confusing, isn't it?
I tried to rework it to be more clear and less verbose.
Compare
For known number of paths
and
For unknown number of paths
and
Uh oh!
There was an error while loading. Please reload this page.
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.
Now that
commitis move todrop, there would be nocommitcall:It seems to me the
commit-less version ofpaths_mutis now just a lower-level version ofupdate_paths. It's now a problem of verbosity rather than inconsistency.I appreciate your effort to make the API more friendly. But personally, I'd always prefer to expose a lower-level API than a friendly API in a rust library. You can always create a
update_pathsfunction externally based onpaths_mutif you feelpaths_mutis too verbose.I know the performance is not really relevant here, but as a Rust developer, it just feels weird to see an API requiring a
Vec, whereaspaths_mutis a commonly known "builder" pattern (example:bindgen::Builder).If you still strongly feel
update_pathsis essential innotify, please at least consider keepingpaths_mutas well for people like me who prefer a builder-style API.cc @dfaust
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.
Builder api is a good choice there and it can be made without lib-support, you can just build a
Vecwith someUpdatePathsBuilderyou made. I think example proves it - they take anyInto<String>into builder methodheader(for example) and makeBox<str>that is added into theVec<Box<str>>.Watcherjust can't take generics because of dyn compability but the effect is the same as in your example - build some in-memory stuff to call the main method (build,update_paths, etc).To be honest,
PathsMutjust pretends to be a builder, but it is not, because it doesn't build anything new, it mutates the watcherYes, but I just try to explain my opinion, that an API that allows user to have notify objects in a state that can't be come in any other ways is not a good choice. The verbosity reduction is just a side effect.
If we remove
committhis state is still there - afterpaths_mutcall. There are some implicit things that may be really confusing, for me it would be "Could I call long-time running code betweenaddcalls?". As a user I wouldn't expect, that I don't get any events afterpaths_mutcall. This argument may work with my API choice either, but at leastupdate_pathsdoesn't allow user to do anything while service is being stopped and forces user to wait the call finishesThere 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.
@branchseer I think about it in a descriptive manner: how I explain that thing to a user.
PathsMutpaths_mutmethod and store the object you got. The method may fail in cases, that depend on the watcher you use (but the cases are not the same as inwatch/unwatchcalls)addand / orremovemethods on the given object. They may fail some cases, that similar to thewatchunwatchbut not the sameupdate_pathsVecofPathOp(PathOp::WatchorPathOp::Unwatch)update_pathsmethod with theVec. The method will fail in the exactly the same cases aswatchandunwatch.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.
That's an interesting approach - it does avoid the main issue of inconsistent watcher state externally.
That said, the resulting API feels a bit unusual.
And the allocation is still present - a
Boxis required either way.Do you think avoiding allocations here justifies making the API substantially less clear? I was under the impression that, compared to filesystem calls, memory allocation was considered negligible.
Uh oh!
There was an error while loading. Please reload this page.
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.
Well the callback style looks very natural to me. Here's a similar API from std:
std::thread::scope.Besides the builder parameter, I like every method returning the same type of plain error that can be easily handled using
?. To meUpdatePathsErroris a high cognitive overhead.This is just expressing my personal state, not to undermine your work :) Really appreciate you putting time and thoughts into this.
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.
Yes, but
thread::scopetakes a generic parameter and you don't have to box itGood point. I've added
impl From<UpdatePathsError> for ErrorYeh, it is, but we do need a way to handle paths, that weren’t successfully added. In some cases if you don't care about errors you can call
update_pathsin a loopwith
Box<dyn FnMut(PathsMut) -> Result<(), Error>>we have to do something like that:No worries at all - I didn’t take it that way.
That said, I feel like in our efforts to improve the
PathsMutAPI, we might be losing some of the broader context. Specifically, beyond just adjusting how we handle paths, we also need to account for other inputs — like supporting not justRecursiveMode, but also more general configuration viaWatchConfig.Changing just
PathsMutwould be a breaking change, so we’ll need to evolve the API structure anyway. As I mentioned earlier, I see a few possible directions to address this.Another option would be:
PathsMuttrait, e.g.watch(&Path, WatchConfig). Since it's not really user-implementable, we can technically do this. But this would still suffer from the same core issue - inconsistency: either methods that never fail but returnResult, or strange pre-commit state. In this case,update_pathsbecomes unnecessary, but to be honest, I'm strongly against this direction - it feels like an awkward and messy API.Box<dyn FnMut(&mut PathsMut) -> Result<(), Error>>. Personally, I find this approach less clear overall. It also wouldn’t solve the inconsistency between callingpaths_mut()andcommit()means we’d still need to deprecateWatcher::paths_muteventually.paths_mutandPathsMutentirely,update_pathstakes aVec<PathOp>. This is what the current PR does. I like this approach best - which is why I implemented it.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 like the callback approach best. It fixes the inconsistency issue and it should work with an implementation of #632, right? It allows error propagation and requires only a single allocation, which is not an issue in our io heavy scenario. It should also allow to optimize the clones it mentioned away (but that's for another day).
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.
Just my two cents: overall I agree with @riberk, the current impl does look like cleaner over the callback one. The error handling with the callback approach looks a deal breaker and wouldn't be user-friendly IMO. I'm also not sure allocation on Box vs. Vec would make a noticable difference here (in general).