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

Support array in default #464

Merged
merged 3 commits into from
Dec 19, 2023
Merged

Conversation

EmilyGraceSeville7cf
Copy link
Collaborator

closes #463

The easiest way to add array support for default.

To implement dependency between repeatable and default properties here if is required, but it means that common properties to then and else branches should be declared inside definitions. Will u accept such change if I make it?

@EmilyGraceSeville7cf EmilyGraceSeville7cf added the enhancement New feature or request label Dec 19, 2023
@EmilyGraceSeville7cf EmilyGraceSeville7cf self-assigned this Dec 19, 2023
@DannyBen
Copy link
Owner

DannyBen commented Dec 19, 2023

I think what you have done here is sufficient, no? I prefer simplicity whenever possible.

Have you figured out how come the check-jsonschema did not fail when only string is allowed, and it was an array?

@EmilyGraceSeville7cf
Copy link
Collaborator Author

EmilyGraceSeville7cf commented Dec 19, 2023

I think what you have done here is sufficient, no? I prefer simplicity whenever possible.

Got it. ;)

Have you figured out how come the check-jsonschema did not fail when only string is allowed, and it was an array?

It may be a bug, I'll check check-jsonschema and some other validators. I see that rush command is used to install check-jsonschema. I guess it is this one, am I right?

@DannyBen
Copy link
Owner

I guess it is this one, am I right?

Nope. It is my own rush, and the installation script for check-jsonschema is in my own rush repo.

I simply install it with pipx (on my arch linux).

$ pipx install check-jsonschema

(sorry - just realized I edited your comment instead of replying so I recreated yours.... 🤦)

@EmilyGraceSeville7cf
Copy link
Collaborator Author

EmilyGraceSeville7cf commented Dec 19, 2023

It seems to be a bug, I have to investigate why it happens and report about it if developers aren't already informed about it. As a temporary solution, I suggest replacing this validator with another one.

(sorry - just realized I edited your comment instead of replying so I recreated yours.... 🤦)

I thought it was a GitHub bug that it showed your reply and my username above it. 😆

@DannyBen
Copy link
Owner

So - is this ready to merge from your standpoint?

@EmilyGraceSeville7cf
Copy link
Collaborator Author

EmilyGraceSeville7cf commented Dec 19, 2023

So - is this ready to merge from your standpoint?

Yep. I suggest create an issue about this CI problem with check-jsonschema not to forget about it until it fixed.

@DannyBen
Copy link
Owner

... and one last thing:

It seems to be a bug, I have to investigate why it happens and report

If you know of a better, or the "de facto standard" CLI for validating JSON schema, let me know and I will switch to it.

@DannyBen
Copy link
Owner

I suggest create an issue about this CI problem with check-jsonschema not to forget about it until it fixed.

Right. I will, once I am also able to create a reproducible simple test case.

@DannyBen
Copy link
Owner

Thanks for your quick implementation, as always.

@DannyBen DannyBen merged commit 9c5eb67 into master Dec 19, 2023
6 checks passed
@DannyBen DannyBen deleted the feat/support-flag-and-arg-array-default branch December 19, 2023 18:08
@DannyBen
Copy link
Owner

DannyBen commented Dec 19, 2023

@EmilyGraceSeville7cf - see #465

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for array in flag.default and arg.default schema
2 participants