-
Notifications
You must be signed in to change notification settings - Fork 70
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: Initial implementation of HTTP connector #1649
base: main
Are you sure you want to change the base?
Conversation
6ab2dc8
to
10ea58f
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1649 +/- ##
==========================================
+ Coverage 89.50% 89.69% +0.18%
==========================================
Files 58 59 +1
Lines 4804 4872 +68
Branches 943 951 +8
==========================================
+ Hits 4300 4370 +70
+ Misses 351 350 -1
+ Partials 153 152 -1 ☔ View full report in Codecov by Sentry. |
89fd553
to
aa916be
Compare
c76de68
to
4fea3f9
Compare
@edgarrmondragon how is this going? I'm just curious because once this merges I can update #1655 to use the same pattern you land on. |
4fea3f9
to
e237760
Compare
1c105be
to
002c4f8
Compare
@pnadolny13 I think the basic approach is ready for review, but I'd like to read some thoughts from other engineers. |
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.
Overall I really like this @edgarrmondragon 🙌 Added a few comments around the Connector API 🙂
singer_sdk/streams/rest.py
Outdated
self.connector = HTTPConnector(self.config) | ||
|
||
# Override the connector's auth with the stream's auth | ||
self.connector._auth = self.authenticator |
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.
Is there a way to make this auth
override part of the connector constructor, rather than manipulating the private attr? Alternatively maybe a helper method to the connector class ?
self.connector._auth = self.authenticator | |
self.connector.override_auth(self.authenticator) |
A public .authenticator
attr could also work (wrapping the existing _auth
private attr), but is maybe less self-documenting 🤔
afee941
to
493ba22
Compare
CodSpeed Performance ReportMerging #1649 will not alter performanceComparing Summary
|
Related:
📚 Documentation preview 📚: https://meltano-sdk--1649.org.readthedocs.build/en/1649/