-
-
Notifications
You must be signed in to change notification settings - Fork 178
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
Wrap arguments into classes #556
Conversation
I haven't fully reviewed this myself, but pinging @iamcarbon fyi. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #556 +/- ##
==========================================
+ Coverage 74.62% 74.72% +0.10%
==========================================
Files 101 103 +2
Lines 2719 2730 +11
Branches 464 464
==========================================
+ Hits 2029 2040 +11
Misses 581 581
Partials 109 109 ☔ View full report in Codecov by Sentry. |
1890086
to
8b457dc
Compare
@iamcarbon Please feel free to feedback on this. |
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.
This is a nice improvement, and gives us some needed flexibility as the Fido2 & WebAuthN specifications continue to evolve.
I don't love the Params postfix, but agree with the reasoning to go against the .NET convention.
Just a couple nits on descriptions, line breaks, and property visibility that can be improved anytime.
Reason: In order to make arguments more readable and future API changes less likely to cause binary breaking changes to everyone we're moving many arguments into wrapping classes.
In this PR the classes are suffixed with Params, instead of popular
Options
/Request
to avoid adding confusion as those terms are used elsewhere as part of webauthn.