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

Add If-Then-Else schema validation. #581

Closed
wants to merge 1 commit into from
Closed

Add If-Then-Else schema validation. #581

wants to merge 1 commit into from

Conversation

niconoe-
Copy link
Contributor

@niconoe- niconoe- commented Aug 7, 2019

Hi,

This is a proposal to be able to understand schema validation with the "if-then-else" schema properties.
Please provide more test cases in the unit test if you want to check more things, but I think all cases are checked here.

Have a nice day.
Regards,

@shmax
Copy link
Collaborator

shmax commented Aug 7, 2019

Isn't this a Json Schema 7 feature? We're still on 5.

@erayd
Copy link
Contributor

erayd commented Aug 7, 2019

@shmax We're still on 4, actually. Version 5 was never actually a thing. But I don't want to turn this down; we're going to need support for more recent versions anyway, and if somebody has the time to help out, then I'm in favour of it provided said help is spec-compliant.

@niconoe- Thank you for this contribution. However, if you want us to merge it, it needs to be fully spec-compliant, and it needs to pass the official test suite, same as we currently do for v3 and v4 - individual custom tests for this specific feature aren't sufficient, and there are a lot more eyes on the official tests to ensure that they are comprehensive and spec-compliant. We'll also need support for schema validation against the v7 meta-schema (or at least the parts of it that pertain to this feature).

@shmax
Copy link
Collaborator

shmax commented Aug 7, 2019

We're still on 4, actually. Version 5 was never actually a thing.

Right. I don't know where I got 5 from. But neither 4 nor 5 are 7, and I figured that gearing up for 7 is going to be a bit more complex than sneaking piecemeal features into the same space as 4.

@erayd
Copy link
Contributor

erayd commented Aug 7, 2019

@shmax Less complicated than you might think, although I agree that your statement definitely applies to full v7 support. But this particular feature is one that can be implemented standalone, and also happens to be useful standalone.

We don't do strict versioning (e.g. you can combine v3 and v4 syntax in a schema); this would behave the same.

@niconoe-
Copy link
Contributor Author

niconoe- commented Aug 8, 2019

@shmax @erayd Thank you for your feedbacks.

Actually, I propose this PR because I needed JSON schema validation on a project of mine and I tried some tools, but this one was the best even if it's not fully schema-draft-07 complient.

I just add the if-then-else management for my business needs and I wanted to participate on providing this PR.
I understand this is draft-07 feature and the support is only draft-04 right now on this project, but I agree @erayd on the fact that managing those draft-07 properties is something that can be implemented standalone. And you might have some other developers like me wanting another partial support from the draft-07, providing a PR to implement it and, step by step, starting to be fully draft-07 complient.

I could try to start validate the "if-then-else" part of the meta-schema-07, but I don't know yet if there's something started on v7 validation. I saw this on v3 and v4 only. If nothing has been done yet, it could pollute this PR as it might be a heavy stuff, don't you think? I'd rather like to provide a v7 meta-schema validation first with only "if-then-else" management on a separate PR. What's your opinion on this?

Beside this, I have 2 questions:

  1. @erayd You said that to be merged, it needs to pass the official test-suite. I completly agree with this as I want to provide a PR with all required quality level. How can I ensure that? Is there a place where tests are already done and waiting for some implementation that will pass them? Where can I find all the tests that the "if-then-else" part must pass?

  2. The checks on the Travis failed for PHP 5.4 and PHP 5.5, but it looks like it's not due to my dev. Travis said phpenv global 5.x failed while it's perfectly working on other PHP versions. Does anyone have an idea why it fails?

Thanks a lot.

EDIT: @erayd After working on #582, I better understand now the test suite you mentionned. It could be painful to validate only some parts of the Draft 07 according to the current PHPUnit TestCase structure you have built, but I will give it a try in a future PR in which I'll include the Draft 07 validation.

blurlabs pushed a commit to Neofonie/json-schema that referenced this pull request Nov 4, 2019
@niconoe-
Copy link
Contributor Author

Closing in favor of #715

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.

3 participants