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.

Sign up for GitHub

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

Jump to bottom

Resolve #698 - Rename requireUserVerification #699

Merged
merged 6 commits into from
Nov 27, 2017
Merged

Resolve #698 - Rename requireUserVerification #699

merged 6 commits into from
Nov 27, 2017

Conversation

jcjones
Copy link
Contributor

@jcjones jcjones commented Nov 24, 2017 •
edited by pr-preview bot
Loading

@ttaubert writes:

I'd like to propose that AuthenticatorSelectionCriteria.requireUserVerification should be renamed to just userVerification. The possible values are "preferred", "required", and "discouraged". Having "require" in the name itself seems misleading. That would also match the change to PublicKeyCredentialRequestOptions.

This PR accomplishes this.
fix #698

Preview | Diff

@jcjones jcjones added this to the WD-07 milestone Nov 24, 2017
equalsJeffH
equalsJeffH reviewed Nov 24, 2017
View reviewed changes
Copy link
Contributor

@equalsJeffH equalsJeffH left a comment

Choose a reason for hiding this comment

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

overall this is fine by me, tho I'm thinking I'd rename requireUserVerification to be userVerificationRequirement

akshayku
akshayku approved these changes Nov 24, 2017
View reviewed changes
Copy link
Contributor

@akshayku akshayku left a comment

Choose a reason for hiding this comment

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

Looks OK to me although I like @equalsJeffH 's suggestion better.

@jcjones
Copy link
Contributor Author

jcjones commented Nov 26, 2017

"userVerificationRequirement is required"
"userVerificationRequirement is preferred"
... doesn't seem better to me.

@emlun
Copy link
Member

emlun commented Nov 26, 2017

In hindsight I agree with @jcjones that it was better before my commits.

@equalsJeffH
Copy link
Contributor

either works for me.

AngeloKai
AngeloKai approved these changes Nov 27, 2017
View reviewed changes
Copy link
Contributor

@AngeloKai AngeloKai left a comment

Choose a reason for hiding this comment

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

Lgtm

@AngeloKai
Copy link
Contributor

@jcjones Please squash and merge before the EOD

@jcjones jcjones merged commit 416732e into master Nov 27, 2017
@jcjones jcjones deleted the 698-rename-UV branch November 27, 2017 20:21
WebAuthnBot pushed a commit that referenced this pull request Nov 27, 2017
Built by Travis-CI: 416732e Resolve #698 - Rename requireUserVerifica…
7f6279f 
…tion (#699)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@equalsJeffH equalsJeffH equalsJeffH left review comments

@akshayku akshayku akshayku approved these changes

@AngeloKai AngeloKai AngeloKai approved these changes

@emlun emlun emlun approved these changes

@balfanz balfanz Awaiting requested review from balfanz

@jyasskin jyasskin Awaiting requested review from jyasskin

Assignees

@jcjones jcjones

Labels
priority:implementation stat:breaking subtype:renaming type:technical
Projects
None yet
Milestone
WD-07
Development

Successfully merging this pull request may close these issues.

Rename AuthenticatorSelectionCriteria.requireUserVerification to userVerification?
5 participants
@jcjones @emlun @equalsJeffH @AngeloKai @akshayku