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.

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

Remove base-class, Status and ErrorMessage #529

Merged
merged 10 commits into from
Jul 16, 2024
Merged

Remove base-class, Status and ErrorMessage #529

merged 10 commits into from
Jul 16, 2024

Conversation

abergs
Copy link
Collaborator

@abergs abergs commented Jul 15, 2024

This PR removes the Fido2ResponseBase-class, which had two fields:

  • Status
  • ErrorMessage

These were never used by the library, only in demoes/samples.

If I recall correctly the reason for these fields are the conformance test tool which looks for them. As such, they should be added in the endpoints that we test against, and not used in the library itself since all errors are handled by throwing exceptions anyway.

Note: This PR is bases for discussion.

@abergs abergs marked this pull request as draft July 15, 2024 14:00
@abergs abergs requested a review from aseigler July 15, 2024 14:00
@abergs
Copy link
Collaborator Author

abergs commented Jul 15, 2024

@aseigler @iamcarbon I would appreciate your input here in case I've missed anything important.

@codecov-commenter
Copy link

codecov-commenter commented Jul 15, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.95%. Comparing base (184f70f) to head (033a3a4).

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #529      +/-   ##
==========================================
- Coverage   74.04%   73.95%   -0.10%     
==========================================
  Files         100       98       -2     
  Lines        2655     2638      -17     
  Branches      446      446              
==========================================
- Hits         1966     1951      -15     
+ Misses        588      586       -2     
  Partials      101      101              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@abergs abergs marked this pull request as ready for review July 15, 2024 15:17
@iamcarbon
Copy link
Contributor

iamcarbon commented Jul 15, 2024

@abergs I had also explored removing the base class during the initial refactoring (they were mixing concerns) -- and agree this is a good time to take the break and move these values to the endpoints.

@abergs
Copy link
Collaborator Author

abergs commented Jul 16, 2024

Alright, I was able to run the conformance tests on this branch and it went fine. Will merge once tests are OK.

@abergs abergs merged commit cb71a15 into master Jul 16, 2024
11 checks passed
@abergs abergs deleted the remove-base branch July 16, 2024 13:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants