-
Notifications
You must be signed in to change notification settings - Fork 0
Sync changes formkeep #2
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
Sync changes formkeep #2
Conversation
…o/tap-formkeep into sync_chenges_formkeep
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.
Pull request overview
This PR refactors the tap-formkeep codebase to use dynamic stream generation instead of static stream classes. The changes replace the hard-coded Submissions stream class with a runtime-generated approach that constructs streams from catalog metadata.
Key Changes:
- Introduced dynamic stream creation via
build_dynamic_stream()function that generates stream classes at runtime - Removed static
Submissionsstream class in favor of dynamic generation - Updated pagination logic to calculate total pages from API response instead of using next-page tokens
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| tap_formkeep/sync.py | Added build_dynamic_stream() function and updated sync() to use dynamic streams |
| tap_formkeep/streams/submissions.py | Removed entire static Submissions stream class |
| tap_formkeep/streams/abstracts.py | Refactored pagination logic and parameter handling to support dynamic streams |
| tap_formkeep/streams/init.py | Removed Submissions import and emptied STREAMS dictionary |
Co-authored-by: Copilot <[email protected]>
| self.metadata = metadata.to_map(catalog.metadata) | ||
| self.child_to_sync = [] | ||
| self.params = {'api_token': 'api_token', 'page': 1, 'page_limit': 25, 'spam': 'False', 'startdate': ''} | ||
| self.params = {'page': 1, 'page_limit': self.page_size} |
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.
Let's make page_size configurable property.
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.
made the change for this
…o/tap-formkeep into sync_chenges_formkeep
…o/tap-formkeep into sync_chenges_formkeep
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.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 5 comments.
tap_formkeep/streams/abstracts.py
Outdated
| url_endpoint = "" | ||
| path = "" | ||
| page_size = 25 | ||
| page = 25 |
Copilot
AI
Nov 28, 2025
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.
The variable name page is misleading as it represents the page size/limit, not a page number. It should be renamed to page_size or default_page_size to accurately reflect its purpose, especially since it's later assigned to self.page_size on line 46.
Description of change
This PR refactors the tap-formkeep codebase to use dynamic stream generation instead of static stream classes.
(SAC-29463)
Manual QA steps
Risks
Rollback steps