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

Make schema type definitions recursive. #464

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Conversation

crobby
Copy link
Contributor

@crobby crobby commented Jan 24, 2025

Issue: #45501

@crobby crobby requested a review from a team as a code owner January 24, 2025 16:55
@crobby crobby marked this pull request as draft January 24, 2025 16:55
@prachidamle prachidamle requested review from ericpromislow and tomleb and removed request for a team January 24, 2025 19:47
@@ -20,23 +20,34 @@ func (s *schemaFieldVisitor) VisitArray(array *proto.Array) {
// it was kept this way to provide backwards compat with previous endpoints.
Copy link
Contributor

Choose a reason for hiding this comment

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

There's something I don't understand about this PR:

  1. The PR title says to make schema type defs recursive
  2. The are no changes in the two functions that do anything recursive -- the only recursive bit is in the description field
  3. Each function has a comment saying : "this currently is not recursive and provides little information for nested types"

I don't understand the problem enough to see which part this PR is actually solving.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair points. I think the word "recursive" may have been misused in the issue related to this PR and then I make it worse by using that word again. I think it's more about accurately reflecting nested definitions. Once I get something slightly stable, I plan to check with the UI team since they're probably the principal consumers of these schemas to see if what I've done is matching what they are expecting.

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