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.

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

Add mapping integrations for taxonomy shopify/2024-07 #372

Merged

Conversation

chesterbot01
Copy link
Contributor

@chesterbot01 chesterbot01 commented Oct 4, 2024

@chesterbot01 chesterbot01 force-pushed the update-directory-folder-to-include-deprecate-taxonomy branch from ba85a31 to c5a1abc Compare October 4, 2024 21:36
@chesterbot01 chesterbot01 force-pushed the update-directory-folder-to-include-deprecate-taxonomy branch 3 times, most recently from 5cf1907 to 8b5549e Compare October 4, 2024 22:33
… ; generate a full_names.yml for the shopify/2024-07 version
@chesterbot01 chesterbot01 force-pushed the update-directory-folder-to-include-deprecate-taxonomy branch from 8b5549e to fbcf91e Compare October 4, 2024 22:43
@@ -139,6 +139,8 @@ def mapping_rules_from(data)
next if integration_id.nil?

raw_mappings = sys.parse_yaml(file)
next if raw_mappings["rules"].nil?
Copy link
Contributor

Choose a reason for hiding this comment

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

nit Would prefer if rules was always an array and we didn't have to do a nil check here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

---
input_taxonomy: shopify/2024-07
output_taxonomy: shopify/2024-10-unstable
rules:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest empty list here so that we don't have to modify the system to allow nil

Suggested change
rules:
rules: []

Comment on lines 152 to 153
next if !taxonomy_version.include?("shopify") || taxonomy_version.include?("shopify/2022-02") ||
taxonomy_version.include?("shopify/2024-07")
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's get a better test here

This will allow shopify/2024-07 to be used as the input/output version of any mapping, which is not what we want. For e.g. in the google/2021-09-21 mapping (from shopify) shopify/2024-07 is not a valid version.

Ok to do this in a follow up PR so we can unblock #356 and #229

Copy link
Contributor Author

@chesterbot01 chesterbot01 Oct 5, 2024

Choose a reason for hiding this comment

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

Good call, I've revised the test to consider taxonomy versions in mapping files as valid if:

  • The source taxonomy version matches the latest Shopify taxonomy version, OR
  • The source taxonomy version is included in the allowlist of Shopify legacy source taxonomies: ["shopify/2022-02", "shopify/2024-07"], AND the destination taxonomy version matches the latest Shopify taxonomy version.

@chesterbot01 chesterbot01 force-pushed the update-directory-folder-to-include-deprecate-taxonomy branch from 0cf07f6 to dcd3f1e Compare October 5, 2024 01:45
@chesterbot01 chesterbot01 force-pushed the update-directory-folder-to-include-deprecate-taxonomy branch from dcd3f1e to eede049 Compare October 5, 2024 01:48
@chesterbot01 chesterbot01 merged commit 9e52faf into main Oct 5, 2024
5 checks passed
@chesterbot01 chesterbot01 deleted the update-directory-folder-to-include-deprecate-taxonomy branch October 5, 2024 01:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants