feat: Introduce function in Tap to convert input catalog to streams#3084
feat: Introduce function in Tap to convert input catalog to streams#3084rafalkrupinski wants to merge 4 commits into
Conversation
Reviewer's GuideThis PR adds a new method to distinguish between discovery and catalog-based execution by introducing Sequence Diagram: Tap.streams Property LogicsequenceDiagram
participant C as Client
participant T as Tap
participant S as Stream
C->>T: Access streams property
T->>T: Check if _streams is initialized
alt "_streams is already initialized"
T-->>C: Return _streams
else "_streams is None"
T->>T: Get input_catalog
alt "input_catalog is None (Discovery Mode)"
T->>T: Call load_streams()
loop "for each discovered stream"
T->>T: Add stream to _streams
end
else "input_catalog is Present (Catalog Mode)"
T->>T: Call load_streams_from_catalog()
loop "for each stream from catalog"
T->>S: stream.apply_catalog(input_catalog)
S-->>T: Catalog applied
T->>T: Add stream to _streams
end
end
T-->>C: Return _streams
end
Class Diagram: Template Tap Inheritance and Method OverrideclassDiagram
class Stream {
// Base Stream class from singer_sdk
}
class CookiecutterStream {
// Represents specific streams like UsersStream, GroupsStream
}
CookiecutterStream --|> Stream
class singer_sdk_Tap {
+load_streams() : list~Stream~
+load_streams_from_catalog() : Iterable~Stream~
}
class TemplateTap {
+input_catalog : Catalog
+load_streams_from_catalog() : Iterable~CookiecutterStream~ // Overridden
}
TemplateTap --|> singer_sdk_Tap
TemplateTap ..> CookiecutterStream : creates
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3084 +/- ##
=======================================
Coverage 92.83% 92.83%
=======================================
Files 63 63
Lines 5427 5430 +3
Branches 672 672
=======================================
+ Hits 5038 5041 +3
Misses 281 281
Partials 108 108
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Hey @rafalkrupinski - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
CodSpeed Performance ReportMerging #3084 will not alter performanceComparing Summary
|
d106420 to
f8f68dd
Compare
|
Cookiecutter check seems to be failing on code that I didn't touch. |
I think it's just complaining about some whitespace. |
edgarrmondragon
left a comment
There was a problem hiding this comment.
hey @rafalkrupinski, thanks for the PR!
Would it make sense to start using the new method for SQL taps, for example?
c0c88c3 to
e6ad82b
Compare
That depends on whether a tap can recreate a stream instance from the catalog entry. On the first glimpse SQLTap can, since it includes sufficient metadata (table, schema name) in the catalog. I'll take a look |
…am objects, without discovery. This clarifies whether the tap should perform discovery or use the input catalog, the two being mutually exclusive.
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
…er.library_name}}/tap.py Co-authored-by: Edgar Ramírez Mondragón <edgarrm358@gmail.com>
…er.library_name}}/tap.py Co-authored-by: Edgar Ramírez Mondragón <edgarrm358@gmail.com>
56aee84 to
e336f31
Compare
Documentation build overview
Files changed
Show files (3) | 3 modified | 0 added | 0 deleted
|
|
Ugh, since this PR is coming from an org GitHub is not letting me push changes 😮💨 |
|
I'm sorry, I won't be able to continue working on this. Do you like to take over or will we just close it? |
|
FWIW, I'm still interested in this |
@ReubenFrankel do you wanna take over from the HEAD of this PR? |
|
@edgarrmondragon will I transfer the repo to you? |
It's not necessary, thanks @rafalkrupinski! Continued: |
Following https://meltano.slack.com/archives/C06A1MD6A6L/p1749119815375359
Introducing
Tap.load_streams_from_catalog(), which should reconstructlist[Stream]from the input catalog.This makes it clear whether the Tap is expected to perform discovery or is run with catalog.
By default, for compatibility, the new function calls
Tap.load_streams(), so it doesn't introduce a breaking change.This is rather trivial function but has some logic, so I could use some hint on what kind of testing would be expected.
Summary by Sourcery
Add a new load_streams_from_catalog method to centralize catalog‐based stream reconstruction and update both the core Tap class and its template to use it without breaking existing behavior.
New Features:
Enhancements:
📚 Documentation preview 📚: https://meltano-sdk--3084.org.readthedocs.build/en/3084/