-
Notifications
You must be signed in to change notification settings - Fork 87
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
imp(02-client): implement TryFrom<TmAbciEvent>
for event types
#1253
Conversation
Hey @rnbguy, could you please review my implementation of |
fn parse_attribute_value<T>( | ||
attribute: &abci::EventAttribute, | ||
f: impl FnOnce(&str) -> Result<T, ClientError>, | ||
) -> Result<T, ClientError> { | ||
match attribute.value_str() { | ||
Ok(value) => f(value), | ||
Err(_) => Err(ClientError::Other { | ||
description: "Invalid attribute value".to_string(), | ||
}), | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can use .map(|value| ...).map_err(|_| ClientError::Other { ... })
directly.
TryFrom<TmAbciEvent>
for event typesTryFrom<TmAbciEvent>
for event types
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1253 +/- ##
==========================================
+ Coverage 67.27% 67.42% +0.14%
==========================================
Files 235 235
Lines 23538 23811 +273
==========================================
+ Hits 15836 16055 +219
- Misses 7702 7756 +54 ☔ View full report in Codecov by Sentry. |
Hey @TropicalDog17, looks like there are a bunch of clippy errors that need addressing. Could you handle those before we circle back for a review on this? 🙏 |
// impl TryFrom<abci::EventAttribute> for ConsensusHeightsAttribute { | ||
// type Error = ClientError; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's remove these commented-out lines
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One tiny nit, but otherwise it looks good to me 👍
Thanks for wrapping up this PR @TropicalDog17!
* add tryfrom for CreateClient event * add try from for other event and attribute * use ClientError::MissingAttributeKey * refactor error type * add tests * linting, refactor complex type * lint * refactor * lint * refactor * add newline * lint
Partially addresses: #1220
Description
PR author checklist:
unclog
.docs/
).Reviewer checklist:
Files changed
in the GitHub PR explorer.