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

Custom error type for Calloop #62

Closed
wants to merge 5 commits into from
Closed

Custom error type for Calloop #62

wants to merge 5 commits into from

Conversation

detly
Copy link
Contributor

@detly detly commented Aug 30, 2021

This is a final-ish draft of creating an error type for Calloop that covers all the major use cases (internal and external). The two things holding it back from not being a draft are:

  • I don't yet know how to exclude necessary parts from tarpaulin
  • I'd like to use it with a couple of my work projects, but I need to port various parts for Calloop's recent changes

Sorry it took so long, it was hard to accommodate certain niche cases. I think I've made it as ergonomic as possible.

The basic gist is:

  • There's a main error type calloop::errors::Error which can take conversions from std::io::Error and anything that converts into Box<dyn Error + Sync + Send>.
  • There's a separate error for InsertError, because it's a bit special but rare.
  • User callbacks or event source code should just be able to use ? and not worry about mapping to and from IO errors etc.

I've used thiserror where possible to remove some boilerplate, it's worked quite nicely but is the source of the tarpaulin issue.

@codecov
Copy link

codecov bot commented Aug 30, 2021

Codecov Report

Merging #62 (c9c63dc) into master (7fd9a0a) will decrease coverage by 20.26%.
The diff coverage is 66.66%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master      #62       +/-   ##
===========================================
- Coverage   80.86%   60.59%   -20.27%     
===========================================
  Files          14       14               
  Lines        1484      873      -611     
===========================================
- Hits         1200      529      -671     
- Misses        284      344       +60     
Impacted Files Coverage Δ
src/sys/kqueue.rs 0.00% <0.00%> (-98.24%) ⬇️
src/error.rs 22.22% <22.22%> (ø)
src/sources/futures.rs 76.19% <50.00%> (-4.41%) ⬇️
src/sys/mod.rs 81.48% <50.00%> (-1.86%) ⬇️
src/sources/ping.rs 82.05% <66.66%> (-6.84%) ⬇️
src/io.rs 59.50% <75.00%> (-7.92%) ⬇️
src/sources/signals.rs 80.00% <80.00%> (ø)
src/loop_logic.rs 70.62% <85.71%> (-9.53%) ⬇️
src/sources/channel.rs 82.05% <100.00%> (+9.55%) ⬆️
src/sources/generic.rs 86.66% <100.00%> (-10.29%) ⬇️
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7fd9a0a...c9c63dc. Read the comment docs.

@elinorbgr
Copy link
Member

So, overall your design looks pretty good, I'll need to read more carefully all the details though.

In the meantime, it appears there are import errors in the src/sys/kqueue.rs file, causing all the freebsd tests to fail, and to exclude code from tarpaulin instrumentation, you can use the #[cfg(not(tarpaulin_include))] annotation on items or blocks that should be ignored.

@detly
Copy link
Contributor Author

detly commented Aug 30, 2021

I just realised that there are some typing issues that still prevent this from working with ? after all. Give me some time to fix that up.

I thought I fixed the kqueue code, I must have broken it in a recent rebase. Cheers.

@detly
Copy link
Contributor Author

detly commented Sep 27, 2021

Closing this PR while I redo some of it.

@detly detly closed this Sep 27, 2021
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

2 participants