-
Couldn't load subscription status.
- Fork 5
Sac 28854 add new metadata changes #25
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: gl-master
Are you sure you want to change the base?
Conversation
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 adds new metadata configuration to the Listrak tap by introducing forced replication methods for all supported streams.
- Adds a
REPLICATION_METHODSdictionary mapping all streams toFULL_TABLEreplication - Creates a
CustomCatalogEntryclass that extends the standardCatalogEntrywith forced replication method support - Updates the discovery process to use the custom catalog entry with predefined replication methods
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tap_listrak/schemas.py | Defines replication methods dictionary mapping all streams to FULL_TABLE |
| tap_listrak/init.py | Implements CustomCatalogEntry class and updates discovery to use forced replication methods |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
tap_listrak/__init__.py
Outdated
| class CustomCatalogEntry(CatalogEntry): | ||
|
|
||
| def __init__(self, *args, forced_replication_method=None, **kwargs): | ||
| self.forced_replication_method = kwargs.pop('forced_replication_method', forced_replication_method) |
Copilot
AI
Oct 9, 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 parameter handling is redundant. The kwargs.pop() call with the same default value as the parameter creates unnecessary complexity. Consider simplifying to self.forced_replication_method = forced_replication_method.
| self.forced_replication_method = kwargs.pop('forced_replication_method', forced_replication_method) | |
| self.forced_replication_method = forced_replication_method |
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.
Take care of this review as well.
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.
added the changes
tap_listrak/__init__.py
Outdated
| def to_dict(self): | ||
| result = super().to_dict() | ||
| if self.forced_replication_method is not None: | ||
| result['forced_replication_method'] = self.forced_replication_method |
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.
| result['forced_replication_method'] = self.forced_replication_method | |
| result['forced-replication-method'] = self.forced_replication_method |
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 changes
tap_listrak/__init__.py
Outdated
| class CustomCatalogEntry(CatalogEntry): | ||
|
|
||
| def __init__(self, *args, forced_replication_method=None, **kwargs): | ||
| self.forced_replication_method = kwargs.pop('forced_replication_method', forced_replication_method) |
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.
Take care of this review as well.
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.
get_standard_metadata is already used in this tap. There was no need to add new class CustomCatalogEntry.
CHANGELOG.md
Outdated
| # Changelog | ||
|
|
||
| ## 1.2.0 | ||
| * forced replication method to metadata [#25](https://github.com/singer-io/tap-listrak/pull/25) |
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.
| * forced replication method to metadata [#25](https://github.com/singer-io/tap-listrak/pull/25) | |
| * Add forced replication method to metadata [#25](https://github.com/singer-io/tap-listrak/pull/25) |
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.
Added minor correction in the changelog, otherwise changes look good so approved the PR. Please fix comment before merging the changes.
Description of change
Add New Metadata to tap
SAC-28854
Risks
AI generated code
https://internal.qlik.dev/general/ways-of-working/code-reviews/#guidelines-for-ai-generated-code