From 5e8b67a60687290c93e2780494fe37558425639a Mon Sep 17 00:00:00 2001 From: Georgi Matev Date: Fri, 30 Sep 2022 15:01:39 -0400 Subject: [PATCH] Explicit ordering of flags based on importance and use frequency (#1002) ## Description Disable automatic flag sorting and explicitly order based on something more sensible. Fixes the order in both inline help output and docs. ## Type of change - [x] :sunflower: Feature - [ ] :bug: Bugfix - [x] :world_map: Documentation - [ ] :robot: Test - [ ] :computer: CI/Deployment - [ ] :hamster: Trivial/Minor ## Issue(s) * Fixes # 783 ## Test Plan - [x] :muscle: Manual - [ ] :zap: Unit test - [ ] :green_heart: E2E --- src/cli/backup/exchange.go | 87 ++++++++++++++++++++----------------- src/cli/cli.go | 5 ++- src/cli/print/print.go | 1 + src/cli/repo/s3.go | 4 +- src/cli/restore/exchange.go | 75 +++++++++++++++++--------------- src/cli/restore/onedrive.go | 5 +++ src/cli/utils/utils.go | 5 ++- 7 files changed, 106 insertions(+), 76 deletions(-) diff --git a/src/cli/backup/exchange.go b/src/cli/backup/exchange.go index 791736283..52aee358d 100644 --- a/src/cli/backup/exchange.go +++ b/src/cli/backup/exchange.go @@ -68,13 +68,16 @@ func addExchangeCommands(parent *cobra.Command) *cobra.Command { switch parent.Use { case createCommand: c, fs = utils.AddCommand(parent, exchangeCreateCmd()) + + // Flags addition ordering should follow the order we want them to appear in help and docs: + // More generic (ex: --all) and more frequently used flags take precedence. + fs.BoolVar(&exchangeAll, + "all", false, + "Backup all Exchange data for all users") fs.StringSliceVar( &user, "user", nil, "Backup Exchange data by user ID; accepts "+utils.Wildcard+" to select all users") - fs.BoolVar(&exchangeAll, - "all", false, - "Backup all Exchange data for all users") fs.StringSliceVar( &exchangeData, "data", nil, @@ -86,20 +89,19 @@ func addExchangeCommands(parent *cobra.Command) *cobra.Command { case detailsCommand: c, fs = utils.AddCommand(parent, exchangeDetailsCmd()) + + // Flags addition ordering should follow the order we want them to appear in help and docs: + // More generic (ex: --all) and more frequently used flags take precedence. fs.StringVar(&backupID, "backup", "", "ID of the backup containing the details to be shown") cobra.CheckErr(c.MarkFlagRequired("backup")) + fs.StringSliceVar( + &user, + "user", nil, + "Select backup details by user ID; accepts "+utils.Wildcard+" to select all users") - // per-data-type flags - fs.StringSliceVar( - &contact, - "contact", nil, - "Select backup details by contact ID; accepts "+utils.Wildcard+" to select all contacts") - fs.StringSliceVar( - &contactFolder, - "contact-folder", nil, - "Select backup details by contact folder ID; accepts "+utils.Wildcard+" to select all contact folders") + // email flags fs.StringSliceVar( &email, "email", nil, @@ -108,24 +110,14 @@ func addExchangeCommands(parent *cobra.Command) *cobra.Command { &emailFolder, "email-folder", nil, "Select backup details by email folder ID; accepts "+utils.Wildcard+" to select all email folders") - fs.StringSliceVar( - &event, - "event", nil, - "Select backup details by event ID; accepts "+utils.Wildcard+" to select all events") - fs.StringSliceVar( - &eventCalendar, - "event-calendar", nil, - "Select backup details by event calendar ID; accepts "+utils.Wildcard+" to select all events") - fs.StringSliceVar( - &user, - "user", nil, - "Select backup details by user ID; accepts "+utils.Wildcard+" to select all users") - - // exchange-info flags fs.StringVar( - &contactName, - "contact-name", "", - "Select backup details where the contact name contains this value") + &emailSubject, + "email-subject", "", + "Restore mail where the email subject lines contain this value") + fs.StringVar( + &emailSender, + "email-sender", "", + "Restore mail where the email sender matches this user id") fs.StringVar( &emailReceivedAfter, "email-received-after", "", @@ -134,14 +126,20 @@ func addExchangeCommands(parent *cobra.Command) *cobra.Command { &emailReceivedBefore, "email-received-before", "", "Restore mail where the email was received before this datetime") + + // event flags + fs.StringSliceVar( + &event, + "event", nil, + "Select backup details by event ID; accepts "+utils.Wildcard+" to select all events") + fs.StringSliceVar( + &eventCalendar, + "event-calendar", nil, + "Select backup details by event calendar ID; accepts "+utils.Wildcard+" to select all events") fs.StringVar( - &emailSender, - "email-sender", "", - "Restore mail where the email sender matches this user id") - fs.StringVar( - &emailSubject, - "email-subject", "", - "Restore mail where the email subject lines contain this value") + &eventSubject, + "event-subject", "", + "Select backup details where the event subject contains this value") fs.StringVar( &eventOrganizer, "event-organizer", "", @@ -158,10 +156,21 @@ func addExchangeCommands(parent *cobra.Command) *cobra.Command { &eventStartsBefore, "event-starts-before", "", "Select backup details where the event starts before this datetime") + + fs.StringSliceVar( + &contact, + "contact", nil, + "Select backup details by contact ID; accepts "+utils.Wildcard+" to select all contacts") + fs.StringSliceVar( + &contactFolder, + "contact-folder", nil, + "Select backup details by contact folder ID; accepts "+utils.Wildcard+" to select all contact folders") + + // exchange-info flags fs.StringVar( - &eventSubject, - "event-subject", "", - "Select backup details where the event subject contains this value") + &contactName, + "contact-name", "", + "Select backup details where the contact name contains this value") case deleteCommand: c, fs = utils.AddCommand(parent, exchangeDeleteCmd()) diff --git a/src/cli/cli.go b/src/cli/cli.go index 53d9fa4ff..2602073c0 100644 --- a/src/cli/cli.go +++ b/src/cli/cli.go @@ -58,11 +58,14 @@ func CorsoCommand() *cobra.Command { // BuildCommandTree builds out the command tree used by the Corso library. func BuildCommandTree(cmd *cobra.Command) { + // want to order flags explicitly + cmd.PersistentFlags().SortFlags = false + cmd.Flags().BoolP("version", "v", version, "current version info") cmd.PersistentPostRunE = config.InitFunc() config.AddConfigFlags(cmd) - print.AddOutputFlag(cmd) logger.AddLogLevelFlag(cmd) + print.AddOutputFlag(cmd) options.AddGlobalOperationFlags(cmd) cmd.CompletionOptions.DisableDefaultCmd = true diff --git a/src/cli/print/print.go b/src/cli/print/print.go index 2a5f3528f..777b188ed 100644 --- a/src/cli/print/print.go +++ b/src/cli/print/print.go @@ -137,6 +137,7 @@ func Item(ctx context.Context, p Printable) { // print prints the printable items, // according to the caller's requested format. +// //revive:disable:redefines-builtin-id func print(w io.Writer, p Printable) { if outputAsJSON || outputAsJSONDebug { diff --git a/src/cli/repo/s3.go b/src/cli/repo/s3.go index f30225d0a..c5c16e95a 100644 --- a/src/cli/repo/s3.go +++ b/src/cli/repo/s3.go @@ -37,10 +37,12 @@ func addS3Commands(parent *cobra.Command) *cobra.Command { c, fs = utils.AddCommand(parent, s3ConnectCmd()) } + // Flags addition ordering should follow the order we want them to appear in help and docs: + // More generic (ex: --all) and more frequently used flags take precedence. fs.StringVar(&bucket, "bucket", "", "Name of the S3 bucket (required).") cobra.CheckErr(c.MarkFlagRequired("bucket")) - fs.StringVar(&endpoint, "endpoint", "s3.amazonaws.com", "Server endpoint for S3 communication.") fs.StringVar(&prefix, "prefix", "", "Prefix applied to objects in the bucket.") + fs.StringVar(&endpoint, "endpoint", "s3.amazonaws.com", "Server endpoint for S3 communication.") fs.BoolVar(&succeedIfExists, "succeed-if-exists", false, "Exit with success if the repo has already been initialized.") // In general, we don't want to expose this flag to users and have them mistake it // for a broad-scale idempotency solution. We can un-hide it later the need arises. diff --git a/src/cli/restore/exchange.go b/src/cli/restore/exchange.go index b691eb4a9..c41ec4f5b 100644 --- a/src/cli/restore/exchange.go +++ b/src/cli/restore/exchange.go @@ -50,18 +50,18 @@ func addExchangeCommands(parent *cobra.Command) *cobra.Command { switch parent.Use { case restoreCommand: c, fs = utils.AddCommand(parent, exchangeRestoreCmd()) + + // Flags addition ordering should follow the order we want them to appear in help and docs: + // More generic (ex: --all) and more frequently used flags take precedence. + // general flags fs.StringVar(&backupID, "backup", "", "ID of the backup to restore") cobra.CheckErr(c.MarkFlagRequired("backup")) - // per-data-type flags - fs.StringSliceVar( - &contact, - "contact", nil, - "Restore contacts by ID; accepts "+utils.Wildcard+" to select all contacts") - fs.StringSliceVar( - &contactFolder, - "contact-folder", nil, - "Restore all contacts within the folder ID; accepts "+utils.Wildcard+" to select all contact folders") + fs.StringSliceVar(&user, + "user", nil, + "Restore all data by user ID; accepts "+utils.Wildcard+" to select all users") + + // email flags fs.StringSliceVar(&email, "email", nil, "Restore emails by ID; accepts "+utils.Wildcard+" to select all emails") @@ -69,22 +69,14 @@ func addExchangeCommands(parent *cobra.Command) *cobra.Command { &emailFolder, "email-folder", nil, "Restore all emails by folder ID; accepts "+utils.Wildcard+" to select all email folders") - fs.StringSliceVar(&event, - "event", nil, - "Restore events by ID; accepts "+utils.Wildcard+" to select all events") - fs.StringSliceVar( - &eventCalendar, - "event-calendar", nil, - "Restore events by calendar ID; accepts "+utils.Wildcard+" to select all event calendars") - fs.StringSliceVar(&user, - "user", nil, - "Restore all data by user ID; accepts "+utils.Wildcard+" to select all users") - - // exchange-info flags fs.StringVar( - &contactName, - "contact-name", "", - "Restore contacts where the contact name contains this value") + &emailSubject, + "email-subject", "", + "Restore mail where the email subject lines contain this value") + fs.StringVar( + &emailSender, + "email-sender", "", + "Restore mail where the email sender matches this user id") fs.StringVar( &emailReceivedAfter, "email-received-after", "", @@ -93,14 +85,19 @@ func addExchangeCommands(parent *cobra.Command) *cobra.Command { &emailReceivedBefore, "email-received-before", "", "Restore mail where the email was received before this datetime") + + // event flags + fs.StringSliceVar(&event, + "event", nil, + "Restore events by ID; accepts "+utils.Wildcard+" to select all events") + fs.StringSliceVar( + &eventCalendar, + "event-calendar", nil, + "Restore events by calendar ID; accepts "+utils.Wildcard+" to select all event calendars") fs.StringVar( - &emailSender, - "email-sender", "", - "Restore mail where the email sender matches this user id") - fs.StringVar( - &emailSubject, - "email-subject", "", - "Restore mail where the email subject lines contain this value") + &eventSubject, + "event-subject", "", + "Restore events where the event subject contains this value") fs.StringVar( &eventOrganizer, "event-organizer", "", @@ -117,10 +114,20 @@ func addExchangeCommands(parent *cobra.Command) *cobra.Command { &eventStartsBefore, "event-starts-before", "", "Restore events where the event starts before this datetime") + + // contacts flags + fs.StringSliceVar( + &contact, + "contact", nil, + "Restore contacts by ID; accepts "+utils.Wildcard+" to select all contacts") + fs.StringSliceVar( + &contactFolder, + "contact-folder", nil, + "Restore all contacts within the folder ID; accepts "+utils.Wildcard+" to select all contact folders") fs.StringVar( - &eventSubject, - "event-subject", "", - "Restore events where the event subject contains this value") + &contactName, + "contact-name", "", + "Restore contacts where the contact name contains this value") // others options.AddOperationFlags(c) diff --git a/src/cli/restore/onedrive.go b/src/cli/restore/onedrive.go index b569f66e9..e36d81204 100644 --- a/src/cli/restore/onedrive.go +++ b/src/cli/restore/onedrive.go @@ -25,6 +25,11 @@ func addOneDriveCommands(parent *cobra.Command) *cobra.Command { switch parent.Use { case restoreCommand: c, fs = utils.AddCommand(parent, oneDriveRestoreCmd()) + + // Flags addition ordering should follow the order we want them to appear in help and docs: + // More generic (ex: --all) and more frequently used flags take precedence. + fs.SortFlags = false + fs.StringVar(&backupID, "backup", "", "ID of the backup to restore") cobra.CheckErr(c.MarkFlagRequired("backup")) diff --git a/src/cli/utils/utils.go b/src/cli/utils/utils.go index 23149b707..74b43cc57 100644 --- a/src/cli/utils/utils.go +++ b/src/cli/utils/utils.go @@ -16,7 +16,7 @@ const ( ) // RequireProps validates the existence of the properties -// in the map. Expects the format map[propName]propVal. +// in the map. Expects the format map[propName]propVal. func RequireProps(props map[string]string) error { for name, val := range props { if len(val) == 0 { @@ -52,5 +52,8 @@ func HasNoFlagsAndShownHelp(cmd *cobra.Command) bool { // and returns both the clone and its pflags. func AddCommand(parent, c *cobra.Command) (*cobra.Command, *pflag.FlagSet) { parent.AddCommand(c) + + c.Flags().SortFlags = false + return c, c.Flags() }