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

decoder/schema: Pass PrefillRequiredFields around via context #230

Merged
merged 6 commits into from
Mar 15, 2023

Conversation

radeksimko
Copy link
Member

@radeksimko radeksimko commented Mar 14, 2023

It was discovered while implementing #203 that schema.Object.EmptyCompletionData() always pre-fills all attributes, which is undesirable.

  • if we need to pre-fill any attributes, it should only be the required ones, not all
  • pre-filling attributes has historically been gated via PrefillRequiredFields, and this should be reflected in objects as well

I also did a little bit of refactoring to (hopefully) make the code more readable, in spite of the additional complexity.

All commits are also part of #203 but I decoupled them here to make it easier to review.


Side note

This will also enable us to later customise the existing literal type/value pre-filling like attr = "value" or attr = false - maybe put it behind a similar feature toggle or do any A/B testing etc.


I wanted to attach some GIFs but this is unfortunately hard to demo without the remaining work in #203

@radeksimko radeksimko self-assigned this Mar 14, 2023
@radeksimko radeksimko marked this pull request as ready for review March 14, 2023 18:46
@radeksimko radeksimko requested a review from a team as a code owner March 14, 2023 18:46
@radeksimko radeksimko changed the title decoder/schema: pass PrefillRequiredFields around via context decoder/schema: Pass PrefillRequiredFields around via context Mar 14, 2023
Copy link
Member

@dbanck dbanck left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Only a small question, maybe we can add a brief comment?

@@ -164,7 +165,8 @@ func requiredFieldsSnippet(bodySchema *schema.BodySchema, placeholder int, inden

var snippet string
if attr.Constraint != nil {
snippet = attr.Constraint.EmptyCompletionData(placeholder, indentCount).Snippet
ctx := schema.WithPrefillRequiredFields(context.Background(), true)
Copy link
Member

Choose a reason for hiding this comment

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

Why are we passing true here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a comment. We could still refactor related functions later and plumb the context all the way through, but I feel it's better not to do it now.

@radeksimko radeksimko merged commit 890a690 into main Mar 15, 2023
@radeksimko radeksimko deleted the f-ctx-prefill-req-fields branch March 15, 2023 08:11
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