-
Notifications
You must be signed in to change notification settings - Fork 22
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
Add --key-config-path
alias
#188
Conversation
Signed-off-by: bgravenorst <[email protected]>
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Signed-off-by: bgravenorst <[email protected]>
@@ -40,7 +40,7 @@ const redocusaurus = [ | |||
|
|||
/** @type {import('@docusaurus/types').Config} */ | |||
const config = { | |||
title: "ConsenSys Web3Signer", | |||
title: "Consensys Web3Signer", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure which way is correct but maybe this should be Web3signer too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should do Web3Signer -> Web3signer in a separate PR? We'll need to ensure the codebase gets updated as well, see: https://github.com/Consensys/web3signer/blob/master/CONTRIBUTING.md
Let me know what you think. I'll gladly fix it up throughout this document if you feel I should go ahead with the changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know which is better/more correct. If there's a requirement to change it though then agree separate PR is best
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will confirm which is the preferred name, and update it (if required) in a seperate PR. Could you please approve the PR @siladu?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bgravenorst just submitted more comments
docs/Reference/CLI/options.md
Outdated
@@ -83,7 +83,7 @@ data-path: "/Users/me/my_node/data" | |||
|
|||
Directory in which to store temporary files. | |||
|
|||
### `key-store-path` | |||
### `key-store-path`, `key-config-path` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
key-config-path is the preferred name so maybe this should come first?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure updating the title is the right thing either. Compare to other options with an alias... https://doc-web3signer-git-fork-bgravenorst-web-185-infura-web.vercel.app/Reference/CLI/options#logging
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll make key-config-path
first (I just copied how it is in the CLI output). We do the aliases for --logging | -l
, --version | -V
, and '--help | -h` differently due to the single dash (-) used by the alias.
But generally we update the title to display the aliases. For example, in Teku.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On a seperate note, shouldn't we also create an alias for --key-store-config-file-max-size
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @bgravenorst that makes sense. Yes, maybe we should create that alias too, good spot, I'll bring it up with the team
Signed-off-by: bgravenorst <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Add
--key-config-path
as a preferred alias forkey-store-path
.Update ConsenSys -> Consensys
fixes #185