Skip to content
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.

Sign up for GitHub

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

Jump to bottom

TDL-6553: Add shop info in record - (Copy of PR 73) #115

Merged
merged 13 commits into from
Oct 5, 2021
Merged

TDL-6553: Add shop info in record - (Copy of PR 73) #115

merged 13 commits into from
Oct 5, 2021

Conversation

savan-chovatiya
Copy link
Contributor

@savan-chovatiya savan-chovatiya commented Aug 5, 2021 •
edited
Loading

Description of change

TDL-6553: Add shop info to the records

QA steps

  • automated tests passing
  • manual qa steps passing (list below)
    • Verified that all three fields related to shop information are emitted for every stream if selected in the catalog.
    • Verified that all three fields related to shop information are skipped if not selected in the catalog

Risks

Rollback steps

  • revert this branch

@savan-chovatiya savan-chovatiya requested review from cosimon and KAllan357 and removed request for KAllan357 and cosimon September 9, 2021 11:47
@savan-chovatiya savan-chovatiya requested review from luandy64 and zachharris1 and removed request for KAllan357 September 23, 2021 10:45
KrisPersonal
KrisPersonal requested changes Sep 29, 2021
View reviewed changes
Copy link
Contributor

@KrisPersonal KrisPersonal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this function add_synthetic_keys_to_schema and hardcode the schema for new SDC fields and make sure that the tests are passing

@savan-chovatiya
Copy link
Contributor Author

Remove this function add_synthetic_keys_to_schema and hardcode the schema for new SDC fields and make sure that the tests are passing

The function add_synthetic_keys_to_schema() is adding _sdc fields to all the streams schema. If we remove it and add those fields directly in schema JSON files then we have to add those fields in newly added streams as well. eg. In newly added 3 streams as part #114 and #118 And if any new streams are added in the future, the developer needs to add these fields to the new stream schema as well.

kspeer825
kspeer825 reviewed Sep 30, 2021
View reviewed changes
Copy link
Contributor

@kspeer825 kspeer825 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good tap-tester test for this change. But may want to consider adding unittests if you are going to stick with this dynamic approach.

dmosorast
dmosorast reviewed Sep 30, 2021
View reviewed changes
Copy link
Contributor

@dmosorast dmosorast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Holding off on approval, since I'm not sure about the big picture around this PR, but I'm okay with the pattern used here.

We like to hard code as much as possible for discoverability/searchability, but the risk of forgetting to add these fields in future streams as the tap is extended is a reasonable point. ✅

@luandy64 luandy64 mentioned this pull request Oct 1, 2021
Closed
2 tasks
@savan-chovatiya
Copy link
Contributor Author

This is a good tap-tester test for this change. But may want to consider adding unittests if you are going to stick with this dynamic approach.

Added unittest for add_synthetic_keys_to_schema() function.

@KrisPersonal KrisPersonal merged commit 69d9f61 into master Oct 5, 2021
@KrisPersonal KrisPersonal deleted the TDL-6553-copy-of-add-shop-info-in-record-73 branch October 5, 2021 16:06
tmck-code pushed a commit to lexerdev/tap-shopify that referenced this pull request Feb 3, 2022
@savan-chovatiya @hpatel41
@luandy64 @tmck-code
TDL-6553: Add shop info in record - (Copy of PR 73) (singer-io#115)
ff4a498 
* TDL:6553- Make copy of PR 73

* Resolved pylint failure

* Added integration test to verify shop info fields are included

* Resolved integration test failure

* Resolved issue for _sdc fields inside nested fields also for some streams

* Resolve pylint error

* Updated integration test

* TDL-6553: made proper types for field

* added unittest

Co-authored-by: Harsh <[email protected]>
Co-authored-by: harshpatel4_crest <[email protected]>
Co-authored-by: Andy Lu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KrisPersonal KrisPersonal KrisPersonal approved these changes

@dbshah1212 dbshah1212 dbshah1212 approved these changes

@luandy64 luandy64 luandy64 approved these changes

@karanpanchal-crest karanpanchal-crest karanpanchal-crest approved these changes

@cosimon cosimon Awaiting requested review from cosimon

@zachharris1 zachharris1 Awaiting requested review from zachharris1

@kspeer825 kspeer825 Awaiting requested review from kspeer825

@dmosorast dmosorast Awaiting requested review from dmosorast

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@savan-chovatiya @karanpanchal-crest @luandy64 @dmosorast @dbshah1212 @kspeer825 @KrisPersonal @hpatel41