Skip to content
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

Add more Debug/Copy trait impls #377

Merged
merged 1 commit into from
Jan 5, 2022
Merged

Add more Debug/Copy trait impls #377

merged 1 commit into from
Jan 5, 2022

Conversation

tmfink
Copy link
Contributor

@tmfink tmfink commented Jan 3, 2022

Also adds other useful impls.

src/config.rs Outdated
@@ -24,7 +24,7 @@ impl RecursiveMode {
/// Runtime configuration items for watchers.
///
/// See the [`Watcher::configure`](../trait.Watcher.html#tymethod.configure) method for usage.
#[derive(Clone, Debug, PartialEq, Eq)]
#[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you need Hash ?

Copy link
Member

@0xpr03 0xpr03 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see why everything has to be Hash and Copy, adding more compile time.
Copy makes everything unable to integrate anything that is clone, so any changes that require clone will be API breaking (especially config is such a candidate). And specifically for watchers it is not advised to implement Clone/Copy.

Also please don't format stuff unrelated to your PR, this makes it much harder to review.

@0xpr03 0xpr03 closed this Jan 3, 2022
@0xpr03 0xpr03 reopened this Jan 3, 2022
@tmfink
Copy link
Contributor Author

tmfink commented Jan 3, 2022

I don't see why everything has to be Hash and Copy, adding more compile time.

By making your types implement common traits (that make sense), it's easier to use the library. Due to orphan rules, it would be impossible for a consumer of the library to put the type in a HashMap or HashSet (without using a newtype wrapper and inventing a hash function). The C-COMMON-TRAITS API guideline has a good write-up.

Also, the difference compile-time is so small it's difficult to measure.

Copy makes everything unable to integrate anything that is clone, so any changes that require clone will be API breaking (especially config is such a candidate).

I'll restrict the Copy impls to the "C-like" enums (no associated data).
Tell me what you think it the next round of review. 😉

Also, it's already a breaking change to change variants of a public enum or to add variants to a non-non_exhausitive public enum (like Config).

And specifically for watchers it is not advised to implement Clone/Copy.

I'll remove the watcher Clone/Copy.
It's nice to get review from someone who knows the codebase better. 😄

Also please don't format stuff unrelated to your PR, this makes it much harder to review.

Sure--I kept them in separate commits, but I'll drop them.
The PR template is a little misleading:

Running `cargo fmt` and/or `cargo clippy` is NOT required but appreciated!

This makes it easier to derive traits when they contain a notify type.
Copy link
Member

@0xpr03 0xpr03 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd remove the debug traits on internal stuff. The changelog addition is nice, though we haven't updated that in a longer time. I'm still fetching all changes manually per release and the "overall" thing will have to be regenerated anyway ;)

src/config.rs Show resolved Hide resolved
src/poll.rs Show resolved Hide resolved
src/poll.rs Show resolved Hide resolved
@0xpr03
Copy link
Member

0xpr03 commented Jan 4, 2022

non-non_exhausitive

True, which is a reason we should probably make the fields non-pub and use a "builder" pattern.

it's easier to use the library

Yes and no. Not adding copy to bigger types prevents you from accidentally copying stuff instead when you'd probably rather use a reference.

@tmfink tmfink requested a review from 0xpr03 January 5, 2022 04:42
@0xpr03
Copy link
Member

0xpr03 commented Jan 5, 2022

Thanks for your patience and the PR! When CI finished I'll merge it.

@0xpr03 0xpr03 merged commit 704e6d6 into notify-rs:main Jan 5, 2022
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