-
Notifications
You must be signed in to change notification settings - Fork 147
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
refactor(code_push_client): create http wrapper to standardize headers #648
Conversation
packages/shorebird_code_push_client/lib/src/code_push_client.dart
Outdated
Show resolved
Hide resolved
packages/shorebird_code_push_client/test/src/code_push_client_test.dart
Outdated
Show resolved
Hide resolved
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #648 +/- ##
============================================
+ Coverage 14.03% 100.00% +85.96%
============================================
Files 4 71 +67
Lines 114 2668 +2554
============================================
+ Hits 16 2668 +2652
+ Misses 98 0 -98
☔ View full report in Codecov by Sentry. |
final request = verify(() => httpClient.send(captureAny())) | ||
.captured | ||
.single as http.BaseRequest; | ||
expect(request.method, equals('GET')); |
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.
Consider using the https://pub.dev/documentation/retrofit/latest/http/HttpMethod-class.html constants here and throughout
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.
Yeah good call will update 👍
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.
That comes from retrofit which isn't a dependency we currently have.
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.
Will merge as is for now but happy to adjust later I just didn't see those constants exported by package:http or dart:io
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.
My mistake! Didn't realize it was from a different package 🤦
Description
Type of Change