-
Notifications
You must be signed in to change notification settings - Fork 335
feat(whoami): return accounts and account ids on whoami #983
Conversation
5a1db3f
to
bec7cb7
Compare
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.
just a couple of things, not much
src/commands/whoami/mod.rs
Outdated
match user { | ||
GlobalUser::TokenAuth { .. } => { |
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.
nit: can we use if let
syntax here instead of match
if let GlobalUser::TokenAuth { .. } = user {
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 the match is more appropriate, fwiw.
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.
it's only matching on one thing, i think generally if let
fits that model better, it doesn't need an else
and would eliminate the _ => ()
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.
can you say more about why you want if let?
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.
(there's no assignment here so i am less inclined)
1be15c2
to
88cbd20
Compare
48750d5
to
5deebd1
Compare
|
||
if let GlobalUser::TokenAuth { api_token: _ } = user { | ||
if accounts.is_empty() { | ||
println!("Your token is missing the 'Account Settings: Read' permission.\n\nPlease generate and auth with a new token that has these perms to be able to list your accounts.\n"); |
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.
accessibility nit: auth -> authenticate, perms -> permissions
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.
also may be worth linking to the api token page if possible https://dash.cloudflare.com/profile/api-tokens
0716475
to
8dca5ad
Compare
8dca5ad
to
5fda862
Compare
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! i think we should do the auto-fill feature in a separate PR
); | ||
message::info(&msg); | ||
println!("\n{} You are logged in with {}.\n", emoji::WAVING, auth,); |
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.
why not use message? it's fine but i've been seeing this a lot lately, maybe our message module is not serving us well.
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.
ur not wrong. as said in air- the table would be offset if i used messaged because it would print INFO.
fixes #630