-
Notifications
You must be signed in to change notification settings - Fork 63
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
Porting the accounts info command to TS #1342
Conversation
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, just a question about behavior and a suggestion for a different type on the builder
|
||
exports.builder = yargs => { | ||
export function builder(yargs: Argv): Argv<AccountInfoArgs> { |
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.
We went back and forth on some ideas for how to type the builder
function in a more useful way, but ultimately we landed on this because it's the simplest option. We can revisit this later on if we feel like we're not getting any value out of it. The main value of TS in these command files will be in the handler methods. We have acceptance tests (coverage will be improving soon) that will catch breaking changes to the builder methods.
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.
A few minor comments, but this all looks good to me!
|
||
const response = await getAccessToken( | ||
personalAccessKey, | ||
personalAccessKey!, |
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.
Should we throw an error if personalAccessKey
isn't in the config? I could go either way
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 think we should, but I'm trying to avoid making any code changes as a part of this port. I can do that as a follow up PR though!
|
||
logger.log(i18n(`${i18nKey}.name`, { name })); | ||
logger.log(i18n(`${i18nKey}.name`, { name: name! })); |
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.
Might just try to find a way to make all the config fields guaranteed to exist (in typescript's eyes) when i redo it
Description and Context
Porting the
hs accounts info
command to TS. I'm only porting a single command in this so we can agree on a pattern for how the commands should look before I port more of them.Screenshots
TODO
Who to Notify