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

Simplify matches #1490

Merged
merged 1 commit into from
Oct 4, 2024
Merged

Simplify matches #1490

merged 1 commit into from
Oct 4, 2024

Conversation

mkeeter
Copy link
Contributor

@mkeeter mkeeter commented Oct 3, 2024

(Staged on top of #1489)

Drive-by cleanup to simplify match statements:

  • Use .. instead of foobar: _ to ignore fields
  • Consolidate match statement with identical branches

(I don't think we're ever deliberately destructuring objects to check their shape, but let me know if that's an incorrect assumption!)

| Message::Unknown(..) => None,

Message::ExtentError { error, .. }
| Message::ErrorReport { error, .. } => Some(error),

Message::ExtentLiveCloseAck { result, .. } => result.as_ref().err(),
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know if this makes it any simpler, but I also don't really care one way or the other. I do think it's all part of your master plan to touch every line in Crucible :)

However, you should be able to put Message::ExtentLiveCloseAck with the rest just below, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know if this makes it any simpler, but I also don't really care one way or the other. I do think it's all part of your master plan to touch every line in Crucible :)

🤫

However, you should be able to put Message::ExtentLiveCloseAck with the rest just below, right?

They're actually different types Result<(), CrucibleError> versus Result<(u64, u64, bool), CrucibleError>, so you can't match on them simultaneously (even though we're only using the error).

Copy link
Contributor

@jmpesp jmpesp left a comment

Choose a reason for hiding this comment

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

same question as Alan, otherwise 🚀

Base automatically changed from mkeeter/complete-job-tracker to main October 4, 2024 19:58
@mkeeter mkeeter merged commit da58373 into main Oct 4, 2024
18 checks passed
@mkeeter mkeeter deleted the mkeeter/simplify-matches branch October 4, 2024 20:33
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.

4 participants