Skip to content
This repository was archived by the owner on Jan 22, 2025. It is now read-only.

Comments

Add keybase username and paper key validation#51

Merged
ddworken merged 3 commits intomasterfrom
david/keybase-username-paperkey-validation
Sep 5, 2019
Merged

Add keybase username and paper key validation#51
ddworken merged 3 commits intomasterfrom
david/keybase-username-paperkey-validation

Conversation

@ddworken
Copy link
Contributor

@ddworken ddworken commented Sep 4, 2019

Validate that:

(username == "" && paperkey == "") || (username != "" && paperkey != "")

In addition, validate that the user can log in if they are supplied.

@ddworken ddworken requested a review from xgess September 4, 2019 14:06
return fmt.Errorf("failed to validate KEYBASE_USERNAME and KEYBASE_PAPERKEY: %v", err)
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

you can probably reorder this big block if you dont want these nested if statements...
i think remove the outer one and then returning nil earlier if they're both blank. whatever. not a blocker.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, but returning nil seems like it would cause issues if someone else was adding more validation later on after this if (which seems likely). So I think best to keep with the pattern of one big if for each thing that is being validated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, but returning nil seems like it would cause issues if more validation was added to the end of this function (which seems likely). So I think probably best to keep with this large if block.

Copy link
Contributor

Choose a reason for hiding this comment

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

it's all a frame of reference. i could just as easily see the validateUsernamePaperkey call be at the bottom as the last thing you might consider doing in a flow of possible validations. no thang.

@ddworken ddworken merged commit a08fe36 into master Sep 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants