Skip to content

fix: Recursive tag application for array fields#101

Merged
samlown merged 3 commits intoinvopop:mainfrom
cloudquery:fix/array-item-tags
Sep 26, 2023
Merged

fix: Recursive tag application for array fields#101
samlown merged 3 commits intoinvopop:mainfrom
cloudquery:fix/array-item-tags

Conversation

@candiduslynx
Copy link
Copy Markdown
Contributor

Prior to this fix minLength would've been missing from the []string field

@candiduslynx
Copy link
Copy Markdown
Contributor Author

@samlown this is one of the things uncovered during the schema gen for CloudQuery.

Copy link
Copy Markdown
Contributor

@samlown samlown left a comment

Choose a reason for hiding this comment

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

It'd be great to just set up a specific test rather than updating the TestUser. There are examples at the bottom of reflect_test.go.

reflect_test.go Outdated
Comment on lines +114 to +115
MinLen []string `json:"min_len" jsonschema:"minLength=2"`
MinVal []float64 `json:"min_val" jsonschema:"minimum=2.5"`
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The way I'm pushing for tests here now is to define a separate and independent test rather than updating the massive TestUser. This reduces the number of updated test JSON files. What that be possible here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done in 6f92551.

Additionally, I removed recursive array ([][]... case) tags application per the impossibility of making decision where the tags belong to. This might lead to issue with [][]string jsonschema:"minLength=1"` not handled properly, but I think it's fine for now.

Copy link
Copy Markdown
Contributor

@samlown samlown left a comment

Choose a reason for hiding this comment

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

LGTM! thanks.

@samlown samlown merged commit 73900b4 into invopop:main Sep 26, 2023
@candiduslynx candiduslynx deleted the fix/array-item-tags branch September 26, 2023 14:39
@candiduslynx
Copy link
Copy Markdown
Contributor Author

@samlown I've stumbled upon an interesting regression from this change, I'll make a follow-up PR (it's related to default behavior being used both for the array & for the underlying string

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