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

Add constructor for DioExceptionType.badCertificate #2174

Merged
merged 3 commits into from
Apr 19, 2024
Merged

Conversation

hgraceb
Copy link
Contributor

@hgraceb hgraceb commented Apr 3, 2024

  • Improve some document comments.
  • Change the parameter position of DioException's constructors to make it most readable.
  • Add constructor for DioExceptionType.badCertificate. There is also one reference in the plugin http2_adapter. But I don't know if changing that with an upgraded version is necessary.
    throw DioException(
    requestOptions: options,
    type: DioExceptionType.badCertificate,
    error: certificate,
    message: 'The certificate of the response is not approved.',
    );

New Pull Request Checklist

  • I have read the Documentation
  • I have searched for a similar pull request in the project and found none
  • I have updated this branch with the latest main branch to avoid conflicts (via merge from master or rebase)
  • I have added the required tests to prove the fix/feature I'm adding
  • I have updated the documentation (if necessary)
  • I have run the tests without failures
  • I have updated the CHANGELOG.md in the corresponding package

Additional context and info (if any)

@hgraceb hgraceb requested a review from a team as a code owner April 3, 2024 13:52
Copy link
Contributor

github-actions bot commented Apr 3, 2024

Code Coverage Report

Package Base Coverage New Coverage Difference
dio 🟢 80.26% 🟢 81.24% 🟢 0.98%
Overall Coverage 🟢 80.26% 🟢 81.24% 🟢 0.98%

Minimum allowed coverage is 0%, this run produced 81.24%

@ueman
Copy link
Contributor

ueman commented Apr 4, 2024

There is also one reference in the plugin http2_adapter. But I don't know if changing that with an upgraded version is necessary.

I'm fine with adding a comment there or creating an issue to fix it later. That way we don't forget about it.

@hgraceb
Copy link
Contributor Author

hgraceb commented Apr 4, 2024

I'm fine with adding a comment there or creating an issue to fix it later. That way we don't forget about it.

I updated and added a TODO comment to do that. What do you think?

+// TODO(EVERYONE): Replace with DioException.badCertificate once upgrade dependencies Dio above 5.4.2.
 throw DioException(
   requestOptions: options,
   type: DioExceptionType.badCertificate,
   error: certificate,
   message: 'The certificate of the response is not approved.',
 );

@kuhnroyal
Copy link
Member

@hgraceb Our pipeline should be fine now. Can you please rebase or merge main?

@hgraceb hgraceb force-pushed the dev branch 2 times, most recently from 57e0678 to 8b078c1 Compare April 19, 2024 05:18
@hgraceb
Copy link
Contributor Author

hgraceb commented Apr 19, 2024

@kuhnroyal Nice works!

@kuhnroyal kuhnroyal merged commit 5e724de into cfug:main Apr 19, 2024
5 of 6 checks passed
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