Fix not: {} schemas when given falsy instances.
#181
Conversation
These were improperly considering objects like None to be valid under
that schema.
The fix here simply adds {} as another special cased schema, since
presumably the `is True` block is meant as an optimization.
Note that there are of course many other things you can give to
`not` which disallow any possible instance, like `not: {"title": "foo"}`
but the fallback block should presumably catch them.
The block that was deleted here never looks correct to me (so I've
simply deleted it). Running the updated test suite would seem to
confirm, as this change takes the suite from:
321 failed, 3268 passed, 12 deselected, 152 xfailed, 140 xpassed
to
309 failed, 3280 passed, 12 deselected, 152 xfailed, 140 xpassed
i.e. a strict increase in compliance.
|
Thank you for your contribution. I see tests failed. Did you check the report? |
|
Yep, as I mentioned, the failures look correct, the suite you have hasn't been updated in quite awhile. |
|
Would you mind updating failing tests as well? |
|
There are 300 of them, which is quite a bit more than I have time to help with :), what resolution were you hoping for for them? Presumably marking them as known failures? That I can possibly find some time for but likely in a week or two. |
|
I don't mean to update all the tests. But to fix failing ones after your changes. https://github.com/bowtie-json-schema/bowtie/actions/runs/7775343265/job/21201221245 |
|
I'm afraid I still don't know what you mean -- that link is a Bowtie test suite run (an old one, where I basically temporarily changed its behavior to avoid tripping the bug this PR fixes). Can you help me understand which failing tests you're referring to? |
These were improperly considering objects like None to be valid under that schema.
The fix here simply adds {} as another special cased schema, since presumably the
is Trueblock is meant as an optimization.Note that there are of course many other things you can give to
notwhich disallow any possible instance, likenot: {"title": "foo"}but the fallback block should presumably catch them.The block that was deleted here never looks correct to me (so I've simply deleted it). Running the updated test suite would seem to confirm, as this change takes the suite from:
to
i.e. a strict increase in compliance.
--
In order to put in this fix I updated your copy of the test suite, which seemed quite out of date.
You can also normally see those results on Bowtie's report for the implementation though actually this bug was discovered (and additional tests added to the test suite) because Bowtie "smoke tests" implementations when it builds them with some simple schemas to ensure basic correctness -- and these schemas are the ones it uses, which actually were incorrectly failing!, hence the PR.
Doing the test suite upgrade leaves lots of failures for other behavior -- I'm not sure what you typically do for that, whether you mark them as
xfailuntil you fix them or whatever.