Add stellar keys use --clear to clear the default identity.
#2332
Conversation
The option on this keys use command doesn't feel natural or intuitive to me. If we look at other commands for inspiration, k8s uses a separate command to clear the selected: What about something like: And then the same with the network: |
| pub name: String, | ||
| pub name: Option<String>, | ||
|
|
||
| /// Clear the default source account. | ||
| #[arg(long)] | ||
| pub clear: bool, |
There was a problem hiding this comment.
Should we omit the clear: bool, and just make it if name is None, then clear it?
| if self.clear && self.name.is_some() { | ||
| return Err(Error::NameWithClear); | ||
| } | ||
|
|
||
| if !self.clear && self.name.is_none() { | ||
| return Err(Error::NameRequired); | ||
| } |
There was a problem hiding this comment.
If we omitted the bool, then we wouldn't need these condition checks.
There was a problem hiding this comment.
Having clap manage this might make it easier:
clap::ArgGroup::new("Key")
.required(true)
.args(& ["name", "clear"]),
|
I don't feel strongly about it. An upside is that the --clear is discoverable inside the help of the command. |
mootz12
left a comment
mootz12
left a comment
There was a problem hiding this comment.
I agree --clear probably isn't the "clear"-est naming. I think the best option would be to just use unset directly.
e.g.
stellar keys unset
stellar network unset
stellar fees unset
Thoughts @fnando @leighmcculloch ?
I'll update the #2321 for fees, maybe we could include network unset in this PR as well.
| if self.clear && self.name.is_some() { | ||
| return Err(Error::NameWithClear); | ||
| } | ||
|
|
||
| if !self.clear && self.name.is_none() { | ||
| return Err(Error::NameRequired); | ||
| } |
There was a problem hiding this comment.
Having clap manage this might make it easier:
clap::ArgGroup::new("Key")
.required(true)
.args(& ["name", "clear"]),
github-project-automation
bot
moved this from Backlog (Not Ready)
to Needs Review
in DevX
|
Oh I like the 'unset' vocab. That's better than 'clear-use'. |
|
Closing this in favor of #2337, which implements |
What
Why
So users have a way to clear the default identity.
Known limitations
N/A