Skip to content

Comments

FileIdCache: Allow flexible handle instead of direct borrow for file ids#664

Merged
dfaust merged 2 commits intonotify-rs:mainfrom
florian-g2:main
Feb 8, 2025
Merged

FileIdCache: Allow flexible handle instead of direct borrow for file ids#664
dfaust merged 2 commits intonotify-rs:mainfrom
florian-g2:main

Conversation

@florian-g2
Copy link
Contributor

Hi,
I suggest changing the return type of the cached_file_id method in the FileIdCache from Option<&FileId> to Option<impl Deref<Target=FileId>>; (breaking change)

This allows trait consumers to choose more freely how to return the FileId.
In a FileIdCache implementation I had to do I was required to return a owned copy of the file id.
The changed return type allows eg. a Cow::Owned or any other new-type implementation that dereferences to a FileId.

This is now doable with the recent MSRV raise to 1.77.

@dfaust
Copy link
Member

dfaust commented Jan 8, 2025

I'm split on this. FileId is just 256 bytes and implements Copy. So maybe returning Option<FileId> would be easiest. But I'm willing to merge it, if you think it's an improvement.
Please update the change-log as well.

@florian-g2
Copy link
Contributor Author

I changed the impl from Deref<Target=FileId> to AsRef<FileId>. The AsRef trait is more correct here.

I see no downside to allow a impl return. They are zero cost at runtime.
Using impl AsRef<FileId> allows the cache to return a owned value, a reference, a wrapper type or even a mutex guard.
In my codebase the FileId is wrapped in a new type to add some functionality, that was the motivation for this change.

As far as I see the change is backwards compatible.
The trait will still allow implementations that return Option<&FileId> for the file id.
Clippy will just show a refining_impl_trait warning.

@florian-g2
Copy link
Contributor Author

Please update the change-log as well.

I am unfamiliar with the process. Should I just add the change at the top of the .md without a version / date?

@dfaust
Copy link
Member

dfaust commented Feb 8, 2025

I am unfamiliar with the process. Should I just add the change at the top of the .md without a version / date?

Yes. Add it under debouncer-full 0.6.0 (unreleased).

@florian-g2
Copy link
Contributor Author

Done. I opted for a simpler description for the change in the changelog. Note that file-id is also touched.

@dfaust dfaust merged commit 1a34031 into notify-rs:main Feb 8, 2025
14 checks passed
@dfaust
Copy link
Member

dfaust commented Feb 8, 2025

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants