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

Make DebouncedEvent be Clone, expose is_recursive #133

Merged
merged 1 commit into from
Nov 22, 2017

Conversation

cmyr
Copy link
Contributor

@cmyr cmyr commented Nov 22, 2017

A speculative PR:

These are two very small changes that would be useful to me. Getting #131 onto crates.io would be appreciated, and if you want to include these that would be additionally so; but I do have some workarounds in place already so this is less important.

If you were going to do a bump for crates.io, and could give an up or down vote on #118 first, it could go in at the same time?

Thanks!

@passcod
Copy link
Member

passcod commented Nov 22, 2017

The Clone derive is fine, but can you explain your use for the expose?

@cmyr
Copy link
Contributor Author

cmyr commented Nov 22, 2017

I have a wrapper API, and I have (perhaps dubiously, I admit) chosen to expose the recursive functionality via a bool, so that consumers don't need to bring notify::RecursiveMode into scope. 🤷‍♂️

@cmyr
Copy link
Contributor Author

cmyr commented Nov 22, 2017

@passcod I'm backtracking, I think I was being too clever. This is now just the Clone.

@passcod passcod merged commit 9956260 into notify-rs:master Nov 22, 2017
@passcod
Copy link
Member

passcod commented Nov 22, 2017

Note for when I cut the release: this is a bump-minor.

@cmyr cmyr deleted the derive_clone branch November 22, 2017 02:42
@michaelfairley
Copy link
Contributor

This doesn't seem to build. 🙁

error[E0277]: the trait bound `Error: std::clone::Clone` is not satisfied
   --> src/lib.rs:415:11
    |
415 |     Error(Error, Option<PathBuf>),
    |           ^^^^^^ the trait `std::clone::Clone` is not implemented for `Error`
    |
    = note: required by `std::clone::Clone::clone`

Add a derive for Clone onto Error results in this, which doesn't seem resolvable:

error[E0277]: the trait bound `std::io::Error: std::clone::Clone` is not satisfied
   --> src/lib.rs:445:8
    |
445 |     Io(io::Error),
    |        ^^^^^^^^^^ the trait `std::clone::Clone` is not implemented for `std::io::Error`
    |
    = note: required by `std::clone::Clone::clone`

@cmyr
Copy link
Contributor Author

cmyr commented Nov 22, 2017

@michaelfairley ugh good catch. I forgot in my workaround I was converting io::Error into a general error.

I don't suppose that's an appropriate approach here. So:

  • We could wrap that error in an Arc
  • We could revert this and let me figure it out on my own

Honestly, the latter might be the better option?

passcod added a commit that referenced this pull request Nov 22, 2017
@cmyr
Copy link
Contributor Author

cmyr commented Nov 22, 2017

@passcod you saw this? I'll put a revert together.

@cmyr
Copy link
Contributor Author

cmyr commented Nov 22, 2017

ah beat me to it, great

@passcod
Copy link
Member

passcod commented Nov 22, 2017

Ah, no, Github's own revert tried to also revert #134, when I just wanted to revert this one. If you can fix before the weekend (when I'll definitely cut a release) don't bother with a revert, though.

@cmyr
Copy link
Contributor Author

cmyr commented Nov 22, 2017

Okay, so I will manually remove this change in a new PR?

passcod added a commit that referenced this pull request Nov 22, 2017
@passcod
Copy link
Member

passcod commented Nov 22, 2017

Hmm, I did the revert. Anyway, you can expect a release this Saturday at the latest.

@cmyr
Copy link
Contributor Author

cmyr commented Nov 22, 2017

@passcod great, thanks for this!

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.

None yet

3 participants