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

standardizing how the account flag is used in our CLI #4736

Closed
wants to merge 1 commit into from

Conversation

patnir
Copy link
Contributor

@patnir patnir commented Feb 14, 2024

Summary

These are the ways we enter the account in our CLI:

  1. (most common) Using the -f flag
  2. As an argument (has it's place, for example when creating an account or changing the name of an account)
  3. -a
  4. --account

As a user want the experience to be as consistent as possible. Provide the account with the same flag. If the account is not provided, use the default account.

This change goes through our CLI commands and attempts to make the way we enter the account consistent.

Testing Plan

Documentation

Does this change require any updates to the Iron Fish Docs (ex. the RPC API
Reference
)? If yes, link a
related documentation pull request for the website.

[ ] Yes

Breaking Change

Is this a breaking change? If yes, add notes below on why this is breaking and
what additional work is required, if any.

[ ] Yes

description: 'The account to send money from',
description: 'The account to send coins from',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

coins is more consistent with the description of the command and of other fields in the command

@patnir patnir marked this pull request as ready for review February 14, 2024 23:12
@patnir patnir requested a review from a team as a code owner February 14, 2024 23:12
@@ -26,7 +26,7 @@ export class PostCommand extends IronfishCommand {
...RemoteFlags,
account: Flags.string({
description: 'The account that created the raw transaction',
required: false,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

by default not required

@danield9tqh
Copy link
Member

What does f stand for? Is that short for from? Seems like that makes sense in some contexts but not others when passing an account. -a seems more universal. was there a reason for going with f instead?

Copy link
Contributor

@hughy hughy left a comment

Choose a reason for hiding this comment

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

I think I might actually prefer to go the opposite way and move towards deprecating single character flags like -f and use the full --account: it's only intuitive to me that f would be an account for send -f(rom)

@patnir
Copy link
Contributor Author

patnir commented Feb 14, 2024

What does f stand for? Is that short for from? Seems like that makes sense in some contexts but not others when passing an account. -a seems more universal. was there a reason for going with f instead?

I wanted to keep send as is since it's probably the most used command and conform everything to that. I agree a is more intuitive

@patnir
Copy link
Contributor Author

patnir commented Feb 14, 2024

I think I might actually prefer to go the opposite way and move towards deprecating single character flags like -f and use the full --account: it's only intuitive to me that f would be an account for send -f(rom)

@hughy i feel like single letter is a common pattern for many CLI interfaces, so i personally don't like the idea of deprecating single characters. I agree that --account is more intuitive. Maybe changing everything to -a would be better like @danield9tqh mentioned above?

@patnir patnir force-pushed the rahul/standardize-account-flag-cli branch from 530e095 to eac885e Compare February 15, 2024 00:17
@andiflabs
Copy link
Contributor

It might be worth to keep (or deprecate, without removing) -f for backward compatibility reasons.

If you want to go the deprecation route, you can add code to detect when it's used and show a warning like "-f is going away in version x.y.z; please switch to -a" (or similar)

@jowparks
Copy link
Contributor

My vote is for keeping -f with deprecation warning, and making -a the universal. I think having single letter flags is useful for power users, having --account is great for newer users.

@patnir patnir force-pushed the rahul/standardize-account-flag-cli branch from eac885e to ee84e91 Compare February 15, 2024 23:59
@patnir patnir requested a review from hughy February 16, 2024 00:16
@patnir
Copy link
Contributor Author

patnir commented Feb 16, 2024

I reverted the change to send, mint, and burn because the -a flag conflicts with the -a flag for amount. I do believe the remaining changes are still relevant and I'll think of what the best approach for the other commands is.

@patnir patnir force-pushed the rahul/standardize-account-flag-cli branch from 6382a84 to e1ddff6 Compare February 16, 2024 01:27
@patnir patnir force-pushed the rahul/standardize-account-flag-cli branch from e1ddff6 to 0938478 Compare February 16, 2024 01:28
@patnir patnir closed this Feb 23, 2024
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.

5 participants