-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Fix --ipns-base alias #7659
Fix --ipns-base alias #7659
Conversation
488c588
to
ed252d4
Compare
ed252d4
to
904ad16
Compare
@@ -1,11 +1,14 @@ | |||
package keyencode |
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.
Any ideas why code coverage is yelling at me here? Is this just because I removed some lines, or is there something sneaky that I've missed? The report looks pretty weird.
const IPNSKeyFormatOptionName = "ipns-base" | ||
const ipnsKeyFormatOptionName = "ipns-base" | ||
|
||
var OptionIPNSBase = cmds.StringOption(ipnsKeyFormatOptionName, "Encoding used for keys: Can either be a multibase encoded CID or a base58btc encoded multihash. Takes {b58mh|base36|k|base32|b...}.").WithDefault("base36") |
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.
@Stebalien Any reason why reusing the same option repeatedly is not recommended?
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.
No, we do that for some shared options. It should be fine.
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.
lgtm
const IPNSKeyFormatOptionName = "ipns-base" | ||
const ipnsKeyFormatOptionName = "ipns-base" | ||
|
||
var OptionIPNSBase = cmds.StringOption(ipnsKeyFormatOptionName, "Encoding used for keys: Can either be a multibase encoded CID or a base58btc encoded multihash. Takes {b58mh|base36|k|base32|b...}.").WithDefault("base36") |
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.
No, we do that for some shared options. It should be fine.
This PR first refactors
--ipns-base
by putting all the options in one location and then removes the empty alias that was being used.