-
Notifications
You must be signed in to change notification settings - Fork 88
feat(taps): Abstract streams #2888
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
base: main
Are you sure you want to change the base?
Conversation
Reviewer's Guide by SourceryThis pull request introduces an Updated class diagram for the Stream classclassDiagram
class Stream {
+selected_by_default: bool
+__abstract__: bool
+__init__(tap: Tap, name: str, schema: dict, ...)
+selected(): bool
}
note for Stream "Added __abstract__ flag and updated selected() method"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov ReportAttention: Patch coverage is
❌ Your patch check has failed because the patch coverage (91.66%) is below the target coverage (100.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #2888 +/- ##
==========================================
+ Coverage 91.93% 91.94% +0.01%
==========================================
Files 62 62
Lines 5295 5302 +7
Branches 681 683 +2
==========================================
+ Hits 4868 4875 +7
Misses 299 299
Partials 128 128 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
CodSpeed Performance ReportMerging #2888 will not alter performanceComparing Summary
|
| raise | ||
|
|
||
| if selected: | ||
| if not self.__abstract__ and selected: |
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.
| if not self.__abstract__ and selected: | |
| if not selected: |
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.
Personally, I would remove the redundant assignment
selected = self.selectedabove for clarity. Then this becomes
| if not self.__abstract__ and selected: | |
| if not self.selected: |
as below.
|
|
||
| # Send a SCHEMA message to the downstream target: | ||
| if self.selected: | ||
| if not self.__abstract__ and self.selected: |
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.
| if not self.__abstract__ and self.selected: | |
| if not self.selected: |
| class MyTap(tap_class): | ||
| def discover_streams(self): | ||
| return [SelectedStream(self), UnselectedStream(self)] | ||
| return [SelectedStream(self), UnselectedStream(self), AbstractStream(self)] |
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.
Would it be possible to avoid this somehow? As in, auto-discover/instantiate any parent streams that are abstract?
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's a very good point. I'll try to do that.
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.
Think this would relate to #953
| return Catalog( | ||
| (stream.tap_stream_id, stream._singer_catalog_entry) # noqa: SLF001 | ||
| for stream in self.streams.values() | ||
| if not stream.__abstract__ |
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.
Advantages/disadvantages of filtering out abstract streams here vs in self.streams?
| def get_records(self, context): # noqa: ARG002 | ||
| yield {"id": 1} | ||
| yield {"id": 2} | ||
|
|
||
| def generate_child_contexts(self, record, context): # noqa: ARG002 | ||
| yield {"pid": record["id"]} |
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.
Wondering if there is a better way to support this... It would be good if this could be encapsulated into generate_child_contexts only, but that currently requires at least one record (hence get_records implementation).
Related
Summary by Sourcery
Add support for abstract streams, which are not included in the catalog.
📚 Documentation preview 📚: https://meltano-sdk--2888.org.readthedocs.build/en/2888/