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

DRAFT: Use thiserror for error types #118

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

miguelfrde
Copy link

@miguelfrde miguelfrde commented Apr 23, 2024

We recently had a need in our codebase to match against an error returned by this crate. Having logic based on various error scenarios at the moment required converting the error to a string and doing string comparisons. This is quite brittle and not ideal.

This is a proposal to return thiserror Errors instead of anyhow Errors in all the crate.

I'm currently taking the approach of converting every instance of anyhow::Error to equivalent variants of various enums deriving thiserror::Error.

I'm currently missing fixing the following directories:

  • tc/*
  • link/*

I'd like to do so in the same PR, but wanted to hear your thoughts on whether this is a direction you like.

This is currently built on top of rust-netlink/netlink-packet-utils#13 We'd need to first land that PR in that crate and publish the changes in a new version of that crate.

This enables downstream clients to be able to match on specific errors
and handle specific error conditions in different ways without having
to compare strings meant for humans reading.
@miguelfrde miguelfrde marked this pull request as draft April 23, 2024 01:54
@miguelfrde miguelfrde changed the title Use thiserror for error types DRAFT: Use thiserror for error types Apr 23, 2024
@daniel-noland
Copy link
Contributor

daniel-noland commented May 2, 2024

I'm new here, but I personally love this idea!

At the same time I am currently working on #111 which adds quite a bit of functionality to the tc/* files you mentioned.

I used anyhow errors a fair amount in that PR because I wanted to keep with the flow of the rest of the code base.

If we are collectively interested in this plan then I can

  1. adapt my flower code to use this if we think this will land before DRAFT: tc flower support #111 goes through
  2. try to push to get DRAFT: tc flower support #111 merged and then make a follow up PR to add structured errors to the new flower code (and the u32 code while I'm at it I suppose)

I'm completely fine with either approach. Just hoping to minimize wasted efforts 😄

Edit:

Looks like I'm slow on the response here and you already started adding error improvements to tc/*

Maybe I should wait for this to land and then rebase?

@cathay4t
Copy link
Member

The error handling should be a architecture enforced to all rust-netlink crates.

Please create pull request to https://github.com/rust-netlink/.github by adding a ADR(Architectural Decision Record) for this error handling. You may find template at https://github.com/rust-netlink/.github/blob/main/architecture_decisions/ADR-000-adr-template.md

For this error handling:

  • I am reluctant to add new dependency, so you have to explain why you think our API stability will not be impacted by breaking changes to anyhow or thiserror. And why thiserror can be trusted(any other rust project using it, any API breaking history)
  • The principle of error handling should be:
    • expandable but API stable
    • Not hiding lower layer error
  • Demo PR like current one proving ADR compliance.

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.

3 participants