Fix for relayer::error displays #559
Conversation
| // TODO: refactor commands: why is chain_id an `Option`? simpler to give it `ChainId` type. | ||
| // see the tx/client.rs or tx/packet.rs for simpler approaches. |
There was a problem hiding this comment.
That's because gumdrop requires types used in options to have a Default instance: https://gist.github.com/romac/23db89d7c5971b0e591a11286fc259a6
While this makes sense for options which are not marked required, it's beyond me why the requirement subsists for required options.
As such, I am not even sure we need the required annotation since we end up checking for the Option to be defined anyway but I guess it does not hurt if the error message thrown by gumdrop is good enough.
There was a problem hiding this comment.
Oh but we have a Default instance for ChainId. Then yeah we don't need the option and can instead just use required.
There was a problem hiding this comment.
We added Default -s for everything when we started to use types in the CLI options. I think we should consider moving to something else in V1 (maybe directly gumdrop) as we need more flexibility.
There was a problem hiding this comment.
As such, I am not even sure we need the required annotation since we end up checking for the Option to be defined anyway but I guess it does not hurt if the error message thrown by gumdrop is good enough.
The error message from gumdrop is a bit.. underwhelming. But it helps still. It says only:
error: missing required free argument
Was looking at ways to parametrize this, but did not find anything.
The way I understand it, with 'required' we now have two levels of checks:
- gumdrop enforces some (any) value to be present
- in our validate_options methods we parse and do additional checks on the field.
We added Default -s for everything when we started to use types in the CLI options. I think we should consider moving to something else in V1 (maybe directly gumdrop) as we need more flexibility.
Sounds like a plan.
There was a problem hiding this comment.
Opened an issue to continue discussion, if necessary, and track this
Codecov Report
@@ Coverage Diff @@
## master #559 +/- ##
=========================================
+ Coverage 13.6% 46.3% +32.6%
=========================================
Files 69 138 +69
Lines 3752 9172 +5420
Branches 1374 0 -1374
=========================================
+ Hits 513 4251 +3738
- Misses 2618 4921 +2303
+ Partials 621 0 -621
Continue to review full report at Codecov.
|
* Include cause in Failed errors (informalsystems#555). Double-checking 'required' tag. * Finished adding 'required' tag to commands * Last nits around error messages. * Changelog * Change doctests in conclude to ignore * Remove Option from chain_id parameter in client queries Co-authored-by: Anca Zamfir <zamfiranca@gmail.com>
requiredtag across all command optionsDescription
For contributor use:
docs/) and code comments.Files changedin the Github PR explorer.