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

Metadata for primitive collection mapping #31351

Merged
merged 3 commits into from
Aug 10, 2023
Merged

Metadata for primitive collection mapping #31351

merged 3 commits into from
Aug 10, 2023

Conversation

ajcvickers
Copy link
Member

Part of #30730

Some stuff remaining, but wanted to get this out there for initial review.

Missing:

  • ElementType in compiled model
  • Negative cases
  • More tests

@ajcvickers ajcvickers requested review from AndriySvyryd and a team July 25, 2023 12:58
@AndriySvyryd
Copy link
Member

Are we going to support nested collections?

@ajcvickers
Copy link
Member Author

Are we going to support nested collections?

We said not now; maybe later.

@AndriySvyryd
Copy link
Member

Are we going to support nested collections?

We said not now; maybe later.

We should at least propose a model building API so that we don't have to make breaking changes.

@ajcvickers ajcvickers force-pushed the 230720Facetbook branch 2 times, most recently from 1a890be to 1e2a724 Compare July 30, 2023 18:29
@ajcvickers
Copy link
Member Author

@AndriySvyryd Updated per API review. Does this shape look correct?

@ajcvickers
Copy link
Member Author

@AndriySvyryd I still have the "ModelBuilding" tests to finish here, but I'm not going to get that done this evening, so I'm sending out for review now since, bugs, negative cases, and testing aside, I think this is ready to merge.

AndriySvyryd
AndriySvyryd requested changes Aug 1, 2023
View reviewed changes
Copy link
Member

@AndriySvyryd AndriySvyryd left a comment

Choose a reason for hiding this comment

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

Also add validation for invalid configuration that's only possible using Metadata API

@ajcvickers
Metadata for primitive collection mapping
a669b39 
Part of #30730

Some stuff remaining, but wanted to get this out there for initial review.

Missing:
- ElementType in compiled model
- Negative cases
- More tests

Review updates

Change builder shape based on API review

Updated based on review.

More tests and refactoring of how ElementTypes are created.
@ajcvickers
Create PrimitiveCollection top-level internal method.
6330937
@ajcvickers
Review tweaks
9d0a4e1
@ajcvickers ajcvickers merged commit 5b556d3 into main Aug 10, 2023
7 checks passed
@ajcvickers ajcvickers deleted the 230720Facetbook branch August 10, 2023 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AndriySvyryd AndriySvyryd AndriySvyryd approved these changes

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

Successfully merging this pull request may close these issues.
2 participants
@ajcvickers @AndriySvyryd