Add CLI command to validate one or all schemas against the strict meta schema

[Vampire]

No description provided.

[github-actions]

Thanks for the PR!

This section of the codebase is owned by @madskristensen and @hyperupcall - if they write a comment saying "LGTM" then it will be merged.

[Vampire]

This PR assumes that #4033 will be accepted.
If #4033 gets rejected, the command in the doc here should be adjusted to be consistent with the others.

[madskristensen]

Awesome. Thank you!

[hyperupcall]

@Vampire Thanks, I think some validation using the strict meta schema is useful, but I think the implementation could be improved. I was thinking that the SchemaName argument probably shouldn't have two different meanings over two different commands. And that the ExplicitTestFile thing should work with both positive and negative tests. There is also a lot of added complexity in the forEachFile, which is supposed to stay as generic and simple as possible.

What do you think about removing the call to taskCheck inside of taskCheckStrict and having a setup similar to this function (without the spinner/strict mode check). You only need to hook into onSchemaFile, and you can modify schema.json.$schema to point to metaschema-draft-07. As long as the $schema is indeed a draft 07, you can call Ajv and handle errors similarly to what's done in other place.

If you don't mind, could you also remove your modifications to forEachFile, including the ExplicitTestFile? I've been thinking about implementing a simpler solution to target individual test files (or multiple schema names) using globs.

[Vampire]

Well, asking for modifications after the PR was merged already is maybe a bit late. :-D

I was thinking that the SchemaName argument probably shouldn't have two different meanings over two different commands

Actually I thought about this when implementing.
And I came to the conlusion, that it's the same meaning.
You specify the schema file that should be handled by the command.
In check all positive and negative test cases are tested against the given schema.
In check-strict the given schema is checked against the meta schema.
But in both cases you supply a schema.
It technically is like copying the given schema to the positive test case directory of the metaschema, removing all other testcases if any present and doing check for the meta schema.

And that the ExplicitTestFile thing should work with both positive and negative tests.

Not sure what you mean.
What negative tests?
The ExplicitTestFile is just an internal implementation detail, thus I also did not add it to readme or help output.
It transports which schema should be verified against the strict meta schema from check-strict to check.
I don't see much sense in specifying a negative test case like that.
If you actually want to test the strict meta schema against positive and negative test cases, put them in the respective test directories.
The check-strict is only to validate other schemas you work on against the meta schema, so that you do not have to copy your schema to the positive test cases of the meta schema and execute check for the meta schema.

What do you think about removing the call to taskCheck inside of taskCheckStrict and having a setup similar to this function

Maybe it is because I'm no a JS dev, but I don't really get what you mean.
Calling check-strict --SchemaName=X really is like calling check --SchemaName=meta with having X as sole positive test case for meta.
So all that is done in check should be done and thus it made sense for me to call check from check-strict.

Of course instead providing some option to specify which schema file to check like check --SchemaName=meta --PositiveTest=X would also be possible, but for me semantically that was then more meta-centric, while from a user-perspective I'm working on X and want to check X against the strict meta schema quickly and conveniently. So from a UX point of view it was more natural for me to have check-strict --SchemaName=X.

[hyperupcall]

Well, asking for modifications after the PR was merged already is maybe a bit late. :-D

@Vampire Yeah, regrettably I wasn't able to comment on this change to cli.js in time

And that the ExplicitTestFile thing should work with both positive and negative tests.

Not sure what you mean.
What negative tests?

Yeah I guess I was a little indirect. What I mean is that I see that ExplicitTestFile is used to change behavior in the forEachFile function. And I'm saying that whatever logic that you have in that function, it should apply to both positive and negative tests. It sort of ties into my point about not modifying forEachFile - it would be preferable to call forEachFile and not use the onNegativeTestFile property at that callsite. It helps keep the code readable.

What do you think about removing the call to taskCheck inside of taskCheckStrict and having a setup similar to this function

Maybe it is because I'm no a JS dev, but I don't really get what you mean.
Calling check-strict --SchemaName=X really is like calling check --SchemaName=meta with having X as sole positive test > case for meta.

I hear you, I understand your justification for making it that way. But I kind of disagree with that approach, and I disagree with how ExplicitTestFile was implemented and how it interacts with SchemaName, and I don't prefer the new semantics of --SchemaName.

I didn't mention this before, but there is also an --unstable-check-with flag, which is supposed to be useful for these slight "variations in check/validation", but I haven't gotten around to implementing it. I think if I have gotten around to doing that, then I think my approach would have been a bit more natural

[hyperupcall]

I appreciate your contribution, and it this PR has some good ideas, but I'm thinking it might be easier if I'd unmerged this, so I can implement this in a different way. I've made some big refactors to cli.js relatively recently, and there are still some things missing, so I apologize if the implementation path wasn't that clear - I'd recommend filing an issue first thing for the next time so this doesn't happen again

[Vampire]

And I'm saying that whatever logic that you have in that function, it should apply to both positive and negative tests. It sort of ties into my point about not modifying forEachFile - it would be preferable to call forEachFile and not use the onNegativeTestFile property at that callsite. It helps keep the code readable.

Sorry, but I still don't see how that should work.
forEachFile only looks into the positive test cases and negative test cases directories for the given schema.
But the the "test-case" in this case is not within those directories.
It practically is an arbitrary file anywhere on disk, in the typical use-case a schema in the src/schemas/json directory.
So I don't see how this should work without a modification to forEachFile, except not using it at all and duplicating all the code.
But as I said, that might also just be my lack of JS expertise.

but there is also an --unstable-check-with flag,

I've seen it, and that it is there but not having any effect so far.
But given the naming, I did not relate it to the use-case I implemented, but thought it is for selecting different validators than Ajv or something like that.

it might be easier if I'd unmerged this, so I can implement this in a different way.

Sure, no worries, I'm not going to be offended, I'm too long active in FOSS for that. :-D
If you see a better way to implement the use-case I welcome you doing that instead.

Just to make it clear again.
My use-case is to

• have src/schemas/json/foo.json
• be able to call a command focused on that schem like npm run check-strict -- --SchemaName=foo.json
• get it validated against src/schemas/json/metaschema-draft-07-unofficial-strict.json by that call

I'd recommend filing an issue first thing for the next time so this doesn't happen again

I had the changes hacked in anyway, as I was fed up of always copying my schema to the positive test dir of the meta schema and then call check for the meta schema. So polishing it slightly and sending a PR was not really much additional work. :-)