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

Fix alignment in help #71

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

vpavlin
Copy link
Member

@vpavlin vpavlin commented Apr 5, 2023

Hi, first of all, I'd like to apologize for unnecessary formatting changes, let me know if you want me to try to avoid those to make the PR simpler.

This PR is an attempt to fix misalignments in help when there are options with and without abbr and various lengths of abbr and when there are long names of options.

I've initially noticed it with wakucanary CLI tool:

 wakucanary [OPTIONS]...

The following options are available:

 -a, --address                 Multiaddress of the peer node to attempt to dial.
 -t, --timeout                 Timeout to consider that the connection failed [=chronos.seconds(10)].
 -p, --protocol                Protocol required to be supported: store,relay,lightpush,filter (can be used
                               multiple times).
 -l, --log-level               Sets the log level [=LogLevel.DEBUG].
 -np, --node-port               Listening port for waku node [=60000].
     --websocket-secure-key-path  Secure websocket key path:   '/path/to/key.txt' .
     --websocket-secure-cert-path  Secure websocket Certificate path:   '/path/to/cert.txt' .

the output looks like this with the change:

The following options are available:

 -a,  --address                     Multiaddress of the peer node to attempt to dial.
 -t,  --timeout                     Timeout to consider that the connection failed.
 -p,  --protocol                    Protocol required to be supported: store,relay,lightpush,filter (can be used
                                   multiple times).
 -l,  --log-level                   Sets the log level.
 -np, --node-port                   Listening port for waku node [=60000].
      --websocket-secure-key-path   Secure websocket key path:   '/path/to/key.txt' .
      --websocket-secure-cert-path  Secure websocket Certificate path:   '/path/to/cert.txt' .

and this is an attempt to fix it and make everything aligned (I am sure it will need iterations).


logLevel* {.
defaultValue: "DEBUG"
desc: "Sets the log level."
name: "log-level" }: string
name: "log-level".}: string

Choose a reason for hiding this comment

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

LGTM! However, why this '.' is needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

This was added by VS code, I am not sure what is "best practice" whether to have the dot there or not - seems to work in both cases:D

Copy link
Member

Choose a reason for hiding this comment

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

we tend to put the dot there consistently

@arnetheduck
Copy link
Member

 -p,  --protocol                    Protocol required to be supported: store,relay,lightpush,filter (can be used
                                   multiple times).

off by one?

@zah
Copy link
Contributor

zah commented Apr 12, 2023

I don't mind formatting fixes in pull requests, but I find the formatting choices being made here to be highly unusual.

Also, I can see that you are planning to use a two-letter abbreviation. I don't think this should be supported by the library, because in the future it intends to add support for the Unix convention of allowing multiple abbreviated options to be concatenated together (e.g. tar -xzfv). Multi-letter abbreviations can lead to ambiguities then.

Can you briefly describe what was the logic problem behind the fixed issue? Is it that the code was not taking the account the presence (or absence) of abbreviations when doing the padding width calculation?

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