-
Notifications
You must be signed in to change notification settings - Fork 247
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
7
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
7 commits
Select commit
Hold shift + click to select a range
aaaac45
feat: rework PathsMut to be more consistent
riberk 7530d19
fix: remove paths mut
riberk f3a13c2
fix: deprecate paths_mut, introduce WatchPathConfig and some renames
riberk 79f2d0e
chore: update changelog for #705
riberk cc1bc73
feat: UpdatePathsError into Error conversion
riberk 1a127c0
fix: typos
riberk 5997e67
fix(test): macos fsevents may return non-canonicalized paths
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
commit
to 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
commit
withdrop
?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
commit
is move todrop
, there would be nocommit
call:It seems to me the
commit
-less version ofpaths_mut
is 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_paths
function externally based onpaths_mut
if you feelpaths_mut
is 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_mut
is a commonly known "builder" pattern (example:bindgen::Builder
).If you still strongly feel
update_paths
is essential innotify
, please at least consider keepingpaths_mut
as 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
Vec
with someUpdatePathsBuilder
you 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>>
.Watcher
just 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,
PathsMut
just 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
commit
this state is still there - afterpaths_mut
call. There are some implicit things that may be really confusing, for me it would be "Could I call long-time running code betweenadd
calls?". As a user I wouldn't expect, that I don't get any events afterpaths_mut
call. This argument may work with my API choice either, but at leastupdate_paths
doesn'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.
PathsMut
paths_mut
method 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
/unwatch
calls)add
and / orremove
methods on the given object. They may fail some cases, that similar to thewatch
unwatch
but not the sameupdate_paths
Vec
ofPathOp
(PathOp::Watch
orPathOp::Unwatch
)update_paths
method with theVec
. The method will fail in the exactly the same cases aswatch
andunwatch
.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.
rolldown doesn't currenly remove paths but yeah dyn safely is a major constraint. How about accepting a callback:
update_watch_paths(&mut self, update_fn: Box<dyn FnOnce(&dyn PathsMut) -> Result<()>>) -> Result<()>
? It will be used like: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
Box
is 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 meUpdatePathsError
is 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::scope
takes a generic parameter and you don't have to box itGood point. I've added
impl From<UpdatePathsError> for Error
Yeh, 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_paths
in 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
PathsMut
API, 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
PathsMut
would 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:
PathsMut
trait, 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_paths
becomes 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_mut
eventually.paths_mut
andPathsMut
entirely,update_paths
takes 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).