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 thiserror to sozu-command-lib #966

Merged
merged 14 commits into from
Aug 2, 2023
Merged

Conversation

Keksoj
Copy link
Member

@Keksoj Keksoj commented Jul 12, 2023

Using anyhow in Sōzu has proven valuable to concatenate context accross function, and display the whole context all in one line for optimal readability.

This readability is only an advantage to humans, though.

We aim to make errors more usable for machines, with proper error enums to be pattern-matched and so on.

@Keksoj Keksoj added this to the v0.16.0 milestone Jul 12, 2023
@Keksoj Keksoj self-assigned this Jul 12, 2023
@Keksoj Keksoj requested a review from Wonshtrum as a code owner July 12, 2023 15:03
@Keksoj
Copy link
Member Author

Keksoj commented Jul 12, 2023

This is to solve this issue:

@Keksoj Keksoj force-pushed the state-dispatch-thiserror branch 6 times, most recently from 971e759 to 7ce7b86 Compare July 20, 2023 09:17
Copy link
Collaborator

@FlorentinDUBOIS FlorentinDUBOIS left a comment

Choose a reason for hiding this comment

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

Hey 👋 , I started the review, but I do not want to create a lot of comments.
Could we avoid to

  • Create items on error that ends with Error, it is redundant (aka ChannerError::FooError)
  • Have generic message error like IoError(std::io::Error) and with a message like read error
  • Create redendundant entries like (ClusterNotFound, ListenerNotFound and so on..), but instead have an error like NotFound { kind: AnEnum, id: String }

command/src/channel.rs Outdated Show resolved Hide resolved
command/src/channel.rs Outdated Show resolved Hide resolved
command/src/channel.rs Outdated Show resolved Hide resolved
command/src/channel.rs Outdated Show resolved Hide resolved
command/src/channel.rs Outdated Show resolved Hide resolved
command/src/config.rs Outdated Show resolved Hide resolved
command/src/scm_socket.rs Outdated Show resolved Hide resolved
command/src/state.rs Outdated Show resolved Hide resolved
command/src/state.rs Outdated Show resolved Hide resolved
command/src/state.rs Outdated Show resolved Hide resolved
@Keksoj
Copy link
Member Author

Keksoj commented Jul 21, 2023

thanks @FlorentinDUBOIS , I removed all ...Error endings in variants, it's more legible 👍 and I merged redundant error types in NotFound or NoMetrics variants.

Copy link
Collaborator

@FlorentinDUBOIS FlorentinDUBOIS left a comment

Choose a reason for hiding this comment

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

Nice works ! There are some stuffs to do, but we are quite close to merge it.

command/src/certificate.rs Outdated Show resolved Hide resolved
command/src/channel.rs Outdated Show resolved Hide resolved
command/src/channel.rs Outdated Show resolved Hide resolved
command/src/channel.rs Outdated Show resolved Hide resolved
command/src/config.rs Outdated Show resolved Hide resolved
lib/src/router/mod.rs Outdated Show resolved Hide resolved
lib/src/socket.rs Outdated Show resolved Hide resolved
lib/src/tls.rs Outdated Show resolved Hide resolved
lib/src/tls.rs Outdated Show resolved Hide resolved
lib/src/tls.rs Outdated Show resolved Hide resolved
@Keksoj Keksoj force-pushed the state-dispatch-thiserror branch 3 times, most recently from 57ed70e to 2ff0385 Compare July 24, 2023 14:47
@Keksoj
Copy link
Member Author

Keksoj commented Jul 25, 2023

this is ready for review BTW

@FlorentinDUBOIS FlorentinDUBOIS merged commit ad53420 into main Aug 2, 2023
12 checks passed
@FlorentinDUBOIS FlorentinDUBOIS deleted the state-dispatch-thiserror branch August 2, 2023 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants