Skip to content

Improve Code Quality #352

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
Merged
merged 4 commits into from
Jan 12, 2023
Merged

Improve Code Quality #352

merged 4 commits into from
Jan 12, 2023

Conversation

iamcarbon
Copy link
Contributor

@iamcarbon iamcarbon commented Dec 7, 2022 •
edited
Loading

This PR:

  • Consolidates the X509Certificate constructor / error handlers in X509CertificateHelper. We now throw an improved error message if there was invalid Base64 data.
  • Uses the built-in JSON converter to decode base64 data. System.Text.Json can convert directly from the utf-8 bytes using the vectorized Base64 converter.
  • Replaces the BinaryWriter with BufferWriter

@codecov-commenter
Copy link

Codecov Report

Merging #352 (d0736bd) into master (b858a5e) will decrease coverage by 0.57%.
The diff coverage is 62.26%.

@@            Coverage Diff             @@
##           master     #352      +/-   ##
==========================================
- Coverage   77.95%   77.38%   -0.58%     
==========================================
  Files          88       89       +1     
  Lines        2513     2520       +7     
  Branches      421      426       +5     
==========================================
- Hits         1959     1950       -9     
- Misses        439      451      +12     
- Partials      115      119       +4
Impacted Files Coverage Δ
...c/Fido2/Metadata/Fido2MetadataServiceRepository.cs 57.57% <41.66%> (-2.62%) ⬇️
...rc/Fido2/Metadata/ConformanceMetadataRepository.cs 88.69% <58.33%> (-1.23%) ⬇️
Src/Fido2/Extensions/JsonElementExtensions.cs 37.50% <60.00%> (-62.50%) ⬇️
Src/Fido2/AttestationFormat/AndroidSafetyNet.cs 96.20% <75.00%> (-2.60%) ⬇️
Src/Fido2/Extensions/X509CertificateHelper.cs 77.77% <77.77%> (ø)
Src/Fido2/AttestationFormat/FidoU2f.cs 91.42% <100.00%> (ø)
Src/Fido2/AttestationFormat/Packed.cs 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@iamcarbon iamcarbon changed the title Improve X509 Certificate construction / error handling Improve Code Quality Dec 7, 2022
@iamcarbon
Copy link
Contributor Author

@aseigler @abergs Ready for review.

@abergs abergs self-requested a review December 8, 2022 08:19
@iamcarbon
Copy link
Contributor Author

@abergs Quick ping for a review.

abergs
abergs approved these changes Jan 11, 2023
View reviewed changes
Copy link
Collaborator

@abergs abergs left a comment

Choose a reason for hiding this comment

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

Sorry that this took time, looks good!

@abergs
Copy link
Collaborator

abergs commented Jan 11, 2023

However tests are failing, while master is green.

Would you like to investigare @iamcarbon?

@aseigler
Copy link
Collaborator

However tests are failing, while master is green.

Would you like to investigare @iamcarbon?

It's because I fixed the errors in a PR submitted after this one.

@iamcarbon
Copy link
Contributor Author

@abergs @aseigler merged in the test fixes from Alex. We're green!

@abergs abergs merged commit 671ecd4 into passwordless-lib:master Jan 12, 2023
@dependabot dependabot bot mentioned this pull request Aug 29, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abergs abergs abergs approved these changes

@aseigler aseigler aseigler approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@iamcarbon @codecov-commenter @abergs @aseigler