Drop placeholders to display default value for flags where it is set. #1207

alexbozhenko · 2024-12-19T20:47:37Z

Dropped from all these lines:

# rg -e  '.Default\(.*.PlaceHolder' -e '.PlaceHolder\(.*Default'

Test plan:

# diff --minimal --context=0 <(nats --help-long) <(go run ./nats --help-long)
*** /dev/fd/63  2025-01-24 14:18:36.058857257 -0800
--- /dev/fd/62  2025-01-24 14:18:36.058857257 -0800
***************
*** 15 ****
!       --connection-name=NAME    Nickname to use for the underlying NATS
--- 15,16 ----
!       --connection-name="NATS CLI Version (devel)"
!                                 Nickname to use for the underlying NATS
***************
*** 24 ****
!       --timeout=DURATION        Time to wait on responses from NATS
--- 25 ----
!       --timeout=5s              Time to wait on responses from NATS
***************
*** 79,82 ****
!     --expire-warn=DURATION  Warn about certs expiring this soon (1w; 0 to
!                             disable)
!     --ocsp                  Report OCSP information, if any
!     --[no-]pem              Show PEM Certificate blocks (true)
--- 80,82 ----
!     --expire-warn=1w  Warn about certs expiring this soon (1w; 0 to disable)
!     --ocsp            Report OCSP information, if any
!     --[no-]pem        Show PEM Certificate blocks (true)
***************
*** 589 ****
!     --backoff-steps=STEPS        Number of steps to use when creating the
--- 589 ----
!     --backoff-steps=10           Number of steps to use when creating the
***************
*** 591 ****
!     --backoff-min=MIN            The shortest backoff period that will be
--- 591 ----
!     --backoff-min=1m             The shortest backoff period that will be
***************
*** 593 ****
!     --backoff-max=MAX            The longest backoff period that will be
--- 593 ----
!     --backoff-max=20m            The longest backoff period that will be
***************
*** 632 ****
!                                  14:18:36)
--- 632 ----
!                                  14:18:37)
***************
*** 653 ****
!         --backoff-steps=STEPS      Number of steps to use when creating the
--- 653 ----
!         --backoff-steps=10         Number of steps to use when creating the
***************
*** 655 ****
!         --backoff-min=MIN          The shortest backoff period that will be
--- 655 ----
!         --backoff-min=1m           The shortest backoff period that will be
***************
*** 657 ****
!         --backoff-max=MAX          The longest backoff period that will be
--- 657 ----
!         --backoff-max=20m          The longest backoff period that will be
***************
*** 741 ****
!     --backoff-steps=STEPS        Number of steps to use when creating the
--- 741 ----
!     --backoff-steps=10           Number of steps to use when creating the
***************
*** 743 ****
!     --backoff-min=MIN            The shortest backoff period that will be
--- 743 ----
!     --backoff-min=1m             The shortest backoff period that will be
***************
*** 745 ****
!     --backoff-max=MAX            The longest backoff period that will be
--- 745 ----
!     --backoff-max=20m            The longest backoff period that will be
***************
*** 784 ****
!                                  14:18:36)
--- 784 ----
!                                  14:18:37)
***************
*** 1184,1193 ****
!     --connect-warn=DURATION      Warning threshold to allow for establishing
!                                  connections
!     --connect-critical=DURATION  Critical threshold to allow for establishing
!                                  connections
!     --rtt-warn=DURATION          Warning threshold to allow for server RTT
!     --rtt-critical=DURATION      Critical threshold to allow for server RTT
!     --req-warn=DURATION          Warning threshold to allow for full round trip
!                                  test
!     --req-critical=DURATION      Critical threshold to allow for full round trip
!                                  test
--- 1184,1191 ----
!     --connect-warn=500ms   Warning threshold to allow for establishing
!                            connections
!     --connect-critical=1s  Critical threshold to allow for establishing
!                            connections
!     --rtt-warn=500ms       Warning threshold to allow for server RTT
!     --rtt-critical=1s      Critical threshold to allow for server RTT
!     --req-warn=500ms       Warning threshold to allow for full round trip test
!     --req-critical=1s      Critical threshold to allow for full round trip test
***************
*** 1240 ****
!     --content=REGEX          Regular expression to check the content against
--- 1238 ----
!     --content=.              Regular expression to check the content against
***************
*** 1413 ****
!         --state=STATE           Limits responses only to those connections that
--- 1411 ----
!         --state=open            Limits responses only to those connections that
***************
*** 1481 ****
!     --filter-state=STATE      Filter on a specific account state (open, closed,
--- 1479 ----
!     --filter-state=open       Filter on a specific account state (open, closed,
***************
*** 1908,1912 ****
!     --[no-]progress     Enables or disables progress reporting using a progress
!                         bar
!     --check             Checks the Stream for health prior to backup
!     --[no-]consumers    Enable or disable consumer backups
!     --chunk-size=BYTES  Sets a specific chunk size that the server will send
--- 1906,1910 ----
!     --[no-]progress       Enables or disables progress reporting using a
!                           progress bar
!     --check               Checks the Stream for health prior to backup
!     --[no-]consumers      Enable or disable consumer backups
!     --chunk-size="128KB"  Sets a specific chunk size that the server will send

alexbozhenko · 2024-12-19T20:48:47Z

@ripienaar WDYT?
Maybe it will be nicer to just change fisk to display both, e.g.
instead of
--backoff-steps=STEPS
do this
--backoff-steps=STEPS(10)
?

ripienaar · 2024-12-20T10:58:46Z

cli/auth_account_command.go

@@ -117,14 +117,14 @@ func configureAuthAccountCommand(auth commandHost) {
 		f.Flag("jetstream", "Enables JetStream").Default("false").IsSetByUser(&c.jetStreamIsSet).BoolVar(&c.jetStream)
 		f.Flag("js-consumers", "Sets the maximum Consumers the account can have").Default("-1").IsSetByUser(&c.maxConsumersIsSet).Int64Var(&c.maxConsumers)
 		f.Flag("js-disk", "Sets a Disk Storage quota").PlaceHolder("BYTES").StringVar(&c.storeMaxString)
-		f.Flag("js-disk-stream", "Sets the maximum size a Disk Storage stream may be").PlaceHolder("BYTES").Default("-1").StringVar(&c.storeMaxStreamString)
+		f.Flag("js-disk-stream", "Sets the maximum size a Disk Storage stream may be").Default("-1").StringVar(&c.storeMaxStreamString)


I dont think universally just updating all of these with a script is the right approach, in the previous one where the default is something useful like 1w it makes sense to show the default, here -1 is more an implementation detail than a useful default users should know about so the placement string should be used in stead.

We will need to evaluate each case and decide if it makes sense to show the default.

A lot of this code was written before fisk has IsSetByUser() and so some of these defaults are there only to compensate for the lack of that feature rather than a default thats useful to the user.

@ripienaar would you be open to a separate PR, where places that use "meaningless" defaults are update to IsSetByUser ?

Yes for now let’s drop placeholders only where it make sense. Later we can move to IsSetByUser()

fixed those

Signed-off-by: Alex Bozhenko <[email protected]>

alexbozhenko · 2025-01-24T22:24:06Z

We can clean up those were both IsSetByUser and default is used separately:
# rg -e '.Default\(.*.IsSetByUser' -e '.IsSetByUser\(.*Default'

E.g. this should show a placeholder, and should not show default, right?

# nats  auth account add -h  | grep 'Maximum allowed connections'
  --connections=-1           Maximum allowed connections

ripienaar

looks good, just the one bit of redundant info we can now remove

ripienaar · 2025-01-27T09:48:02Z

cli/account_tls_command.go

@@ -40,7 +40,7 @@ func configureAccountTLSCommand(srv *fisk.CmdClause) {
 	c := &ActTLSCmd{}

 	tls := srv.Command("tls", "Report TLS chain for connected server").Action(c.showTLS)
-	tls.Flag("expire-warn", "Warn about certs expiring this soon (1w; 0 to disable)").Default("1w").PlaceHolder("DURATION").DurationVar(&c.expireWarnDuration)
+	tls.Flag("expire-warn", "Warn about certs expiring this soon (1w; 0 to disable)").Default("1w").DurationVar(&c.expireWarnDuration)


Lets make this Warn about certs expiring this soon, 0 to disable

ripienaar reviewed Dec 20, 2024

View reviewed changes

drop

00d2d2b

Signed-off-by: Alex Bozhenko <[email protected]>

alexbozhenko force-pushed the drop_placeholders branch from 6b9ead7 to 00d2d2b Compare January 24, 2025 21:51

alexbozhenko added 3 commits January 24, 2025 14:04

revert back meaningless defaults

4abacb2

revert those where IsSetByUser is used

e209ddf

one more

2f0b21a

alexbozhenko marked this pull request as ready for review January 24, 2025 22:20

alexbozhenko requested a review from ripienaar January 24, 2025 22:24

ripienaar requested changes Jan 27, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Drop placeholders to display default value for flags where it is set. #1207

Drop placeholders to display default value for flags where it is set. #1207

alexbozhenko commented Dec 19, 2024 •

edited

Loading

alexbozhenko commented Dec 19, 2024

ripienaar Dec 20, 2024

alexbozhenko Dec 20, 2024

ripienaar Dec 21, 2024

alexbozhenko Jan 24, 2025

alexbozhenko commented Jan 24, 2025

ripienaar left a comment

ripienaar Jan 27, 2025

Drop placeholders to display default value for flags where it is set. #1207

Are you sure you want to change the base?

Drop placeholders to display default value for flags where it is set. #1207

Conversation

alexbozhenko commented Dec 19, 2024 • edited Loading

Test plan:

alexbozhenko commented Dec 19, 2024

ripienaar Dec 20, 2024

Choose a reason for hiding this comment

alexbozhenko Dec 20, 2024

Choose a reason for hiding this comment

ripienaar Dec 21, 2024

Choose a reason for hiding this comment

alexbozhenko Jan 24, 2025

Choose a reason for hiding this comment

alexbozhenko commented Jan 24, 2025

ripienaar left a comment

Choose a reason for hiding this comment

ripienaar Jan 27, 2025

Choose a reason for hiding this comment

alexbozhenko commented Dec 19, 2024 •

edited

Loading