Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 support for
anyOf
toskip_prompt_if
#1133Add support for
anyOf
toskip_prompt_if
#1133Changes from 10 commits
04946d2
6a8c052
5a37d66
f31191f
77116cf
699f7dd
769d092
80cf44b
80156a7
694af22
8ea3f67
89db29a
e52ac4a
2e1ee60
1aa9092
9ee0e3a
daf785d
1a3b8ad
b79a70b
2ed579a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
validateConst
function expectss.Properties
to be non-nil. This is not guaranteed (I think), looking at the JSON schema spec. I understand we're only interested in validating the full config object (i.e. a string/string map) for the skip-prompt-if functionality here, but this seems brittle.Having the
anyOf
property in the schema invites folks to use it as the JSON spec intends (i.e. also at the single field level) and that would cause panics here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it'll panic right? Maybe I am missing what you are pointing to, but a single field
anyOf
will simply be ignored in the current implementation.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the following (valid) example, the
properties
field is not set, and the code will panic.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tried out setting
anyOf
like this, but it does not panic.Accessing empty maps does not panic. You can try this out in the go playground. This does not panic:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, TIL!
Looked into the spec and it says:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Did not know this :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do all these tests run both
validateConst
andValidateInstance
? Looks very repetitive.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This follows the pattern used in the rest of the file, so no need to address in this PR.
Could you take a look at this though?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was intentional. At the cost of being very repetitive, we ensure that the main public API exposed ie
ValidateInstance
and the specific validation rulevalidateConst
are correct. This works here because validations in JSON schema are additive.Are there alternative approaches you would recommend here? Maybe only testing
ValidateInstance
orvalidateConst
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does testing the unexported function add fidelity to the test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what fidelity means in this context, but testing the unexported func does provide information about where the error comes from. Each unexported function here maps to a rule in JSON schema validation (const vs anyOf for example), and confirming the following seems helpful to me:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see the flip side however that unexported functions are not part of the public API. We are good to go as long as the public API return's the right errors.
It does make maintain this easier.
Happy to remove testing for the unexported function.