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 Category loading from YAML source with validation #449

Open
wants to merge 1 commit into
base: dg/refactor-modelindex
Choose a base branch
from

Conversation

danielpgross
Copy link
Collaborator

@danielpgross danielpgross commented Nov 18, 2024

Similar to #443, but for categories: this PR adds the ability to load Category objects from a YAML source with validation.

This implementation fully supports secondary paths to a given category defined using a secondary_children key in the YAML source containing an array. Example:

- id: aa
  name: Apparel & Accessories
  children:
  - aa-1
  attributes:
  - color
- id: aa-1
  name: Clothing
  children: []
  secondary_children:
  - bi-19-10
- id: bi
  name: Business & Industrial
  children:
  - bi-19
  attributes:
  - color
- id: bi-19
  name: Medical
  children:
  - bi-19-10
  attributes:
  - color
- id: bi-19-10
  name: Scrubs
  children: []
  attributes:
  - color

The approach to loading categories is also different from the prototype: load_from_source takes the parsed YAML objects for all verticals (not just one). This was needed because:

  • We need to be able to resolve secondary paths, which can run through a different vertical
  • We need to check uniqueness across all verticals

Copy link
Collaborator Author

danielpgross commented Nov 18, 2024

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

Comment on lines +154 to +155
[parent] + parent.ancestors
end
Copy link

Choose a reason for hiding this comment

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

The ancestors array is being built in the wrong order. The current code returns ancestors from child to root, but the tests expect root to child order. Change to parent.ancestors + [parent] to match test expectations.

Spotted by Graphite Reviewer

Is this helpful? React 👍 or 👎 to let us know.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This one's not correct, the tests expect child to root order. (This line was copied from the existing code to keep existing behaviour.)

@danielpgross danielpgross changed the title Add Category#load_from_source Add Category loading from YAML source with validation Nov 18, 2024
@danielpgross danielpgross changed the base branch from main to dg/refactor-modelindex November 18, 2024 20:32
@danielpgross danielpgross marked this pull request as ready for review November 18, 2024 20:44
# Iterate over the category and all its descendants
#
# @yield [Category]
def traverse(&block)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I assume usage of this will come in later PRs?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For now I've been using it for debugging, ex. for recursively printing out the taxonomy

Copy link
Collaborator

@elsom25 elsom25 left a comment

Choose a reason for hiding this comment

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

🙌

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.

2 participants