RagnarGrootKoerkamp / BAPCtools

Tools for developing ICPC-style programming contest problems.
GNU General Public License v3.0
49 stars 20 forks source link

Remaining TODOs for CUE schemas #301

Closed RagnarGrootKoerkamp closed 7 months ago

RagnarGrootKoerkamp commented 1 year ago
thorehusfeldt commented 1 year ago

I think bt validate should do all the work. The error messages from CUE are not useful and only spread fear; also, there are some things not caught by the schema, off the top of my head:

  1. In #testgroup, at least one of generate, copy, or in must exist.
  2. include can only include names that are elsewhere(? earlier in the inorder traversal?) defined
thorehusfeldt commented 1 year ago

Pr #302 has a an improved JSON schema.

My understanding that one can submit this to schemastore.org and then editors like VS Code just find it.

thorehusfeldt commented 1 year ago

This can probably be closed. What I don’t want to forget:

  1. generators.yaml ought to demonstrate include . There is quite a lot to be said here – including a testcase, including a testgroup, what happens with autonumbering? – so it’s not easy to write.
  2. Consider introducing a version number for the generators framework; my feeling is that we’re at 0.9 or 1.0.0-rc1 some somehing.
  3. One of the nicest part of BAPCtools is validation of invalid inputs; I think this deserves to be visible in generators.yaml as well. (But this requires an internal name-change; I suggest BAPCtools continues to recognise data/bad, possibly with a warning, but otherwise invalid_inputs is indeed the better name. This is drudgery – old example problems need to be renamed, as well as skeletons.)

For point 3, maybe not so bad after all:

> ls **/data/bad
skel/problem/data/bad:
01_empty.in            02_newline.in          03_random_printable.in 04_not_printable.in    05_not_printable.in    06_unicode.in

test/problems/identity/data/bad:
1.in  2.in  3.in  4.ans 4.in  5.ans 5.in  6.ans 6.in  7.in  8.in  9.in
> grep bad **/*.yaml
thorehusfeldt commented 1 year ago

For hosting, I suggest the following:

  1. Consider renaming the schema to generators.json or maybe even problem_package_generators.json.
  2. Decide on a version number. I suggest 0.9 (with the goal of moving to 1.0 after NWERC and WCFD.)
  3. Make the schema known to schemastore. Explanation: https://github.com/SchemaStore/schemastore/blob/master/CONTRIBUTING.md#how-to-add-a-json-schema-thats-self-hostedremoteexternal . I think we need something like the data below.
  4. Wait for the PR to be merged, and hey presto! Several IDEs now find the schema out-of-the-box.
{
  "name": "Problem Package Generators"
  "description": "JSON schema for problem package generator YAML files",
  "fileMatch": ["generators.yaml"],
  "url": "https://raw.githubusercontent.com/RagnarGrootKoerkamp/BAPCtools/master/support/schemas/generators_yaml_schema.json"
}
thorehusfeldt commented 1 year ago

I’ve opened a PR at https://github.com/SchemaStore/schemastore/pull/3196#issue-1885727606 to get the JSON schema hosted. (Ideally, that would mean that somebody who opens a generators.yaml in, say, VS Code, automatically validates against that schema without any local configuration.)

RagnarGrootKoerkamp commented 1 year ago

Sweet! I think we only support the .yaml version but also having .yml is probably good. You could possibly be more strict and only do generators/generators.yaml?

thorehusfeldt commented 1 year ago

I actually thought about restricting to generators/generators.yaml, but already the negative testcases in

https://github.com/thorehusfeldt/schemastore/tree/master/src/negative_test/problem_package_generators

require more naming flexibility, like missing_secret.generators.yaml

(Otherwise I can’t have them in the same directory.)

thorehusfeldt commented 1 year ago

It’s merged and seems to work. This means that we don’t need to document much.

RagnarGrootKoerkamp commented 1 year ago

So awesome! Thanks for the effort! We should totally do this for all the yaml files in the spec once we stabilize them. I'll try it with the LSP Emacs uses soon.

On Thu, 7 Sept 2023, 18:07 Thore Husfeldt, @.***> wrote:

It’s merged and seems to work. This means that we don’t need to document much.

— Reply to this email directly, view it on GitHub https://github.com/RagnarGrootKoerkamp/BAPCtools/issues/301#issuecomment-1710575699, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABPR4UXCLKRDFBJ4MX7E7YDXZIEMLANCNFSM6AAAAAA4ITEWTE . You are receiving this because you authored the thread.Message ID: @.***>

mpsijm commented 1 year ago

I fired up my IntelliJ, and suddenly saw some warnings, so looks like publishing the schema works! :smile:

I found a small bug though: when leaving solution: empty, it shows a warning, while I thought it shouldn't :slightly_smiling_face:

image

The warning at "1": is probably intended :smile:


EDIT: Also, it looks like the interaction: key is not allowed (the counterpart to ans: for sample cases, when working with an interactive problem).

thorehusfeldt commented 1 year ago
  1. null was disallowed from solution in https://github.com/RagnarGrootKoerkamp/BAPCtools/commit/703a9c21aaf4b489bc801450aca629b0ec52c4f1 . Feel free to re-open that discussion here
  2. you are right about interaction.

For 2, should this just be part of #testcase such as:

#testcase:
    // […]
    {
    // […]
        ["in" | "ans" | "desc" | "hint" | "interaction"]: string
    // […]
    }

Or do we know more about interaction strings? (Such as a regex involving < and >?)

mpsijm commented 1 year ago
  1. Ah, fair point! In that case, both warnings were related, but I forgot :smile: Indeed, I don't need the empty solution: key anymore after specifying the ans or interaction file :slightly_smiling_face:
  2. Yes, every line should start with the regex [<>] (note there should be no space after the angle brackets), but I think that should be it :slightly_smiling_face:
thorehusfeldt commented 1 year ago

Let’s see if we can clobber together the interaction regex here. (I’m not sure this is worth doing, but let’s try.)

Here it is in perl-style with non-important white space and comments for readbility

^                # start with
([<>][^\n]*\n)* # lines starting with < or > ending in \n
[<>][^\n]*(\n)? # line starting with < or >, possibly ending in \n
$                # end

Is there another way of doing it? Something that expresses “cannot contain \n followed by anything else than < or >” using ?!?

RagnarGrootKoerkamp commented 1 year ago

First, super cool to find out that this also just works in emacs with my LSP (after turning on LSP-mode in the first place)!

  1. I'd say .interaction files should always end with a newline. BAPCtools already enforces this for hardcoded in/ans/interaction files here.
  2. visualizer: /visualizer/vis.cpp {name} was the typical use case of visualizers, where {name} is replaced as the basename of the testcase being visualized. That's now hardcoded to testcase (in some tmpdir), so {name} isn't needed anymore, but it may still be nice to allow passing in arguments to the visualizer (ie for different visualizer settings per testgroup). The current json spec does not allow this anymore though and requires it to be only an absolute path to a vizualizer program.
  3. I now have autocomplete for known field names, but gitignore_generated isn't known. It used to be out-of-spec, but now that BAPCtools owns the spec we should include it. (But either way the gitignore situation needs some cleaning up now; I'll make a new issue, #305)
thorehusfeldt commented 1 year ago

I can see

[in | ans | desc | hint ] : string
interaction =~ ^([<>][^\n]*\n)+$

or just

[in | ans | desc | hint | interaction] : string

I have no strong opinion either way. (The former seems to use a lot of specification bandwith for a very rare situation.) If somebody feels more strongly, react with tadaa! for the former, eyes for the latter.

thorehusfeldt commented 1 year ago

OK, decided. The changes to the schema is:

        "interaction": {
                "type": "string",
                "title": "Interaction Example",
                "description": "An example interaction.",
                "pattern": "^([<>][^\\n]*\\n)+$"
            },

commit on my own schemastore fork is here: https://github.com/SchemaStore/schemastore/commit/497898c7c39a06e1840d4490bf841f8c242aec51#diff-8a75c0a55e1c4e3d26cee5a1e2a9264b75b3c6d828741ce45973efafc5d44401

Now I “just” need to create a pull request for this to the schemastore, which I find somewhat arduous.

(I’m not sure what the best way of doing this is. Currently we have versions of the schema in the BAPCtools repo, in my schemastore fork, and on the schemastore origin. The tests are run in schemastore. There is away to remotely host a schema. I have no strong opinions.)

mpsijm commented 1 year ago

@thorehusfeldt It looks like you currently made the interaction: field mandatory, as I get a CI failure even though doc/generators.yaml has no interaction keys: https://github.com/RagnarGrootKoerkamp/BAPCtools/actions/runs/6171051643/job/16748536679

Good to see that the CI picks this up though :smile:

RagnarGrootKoerkamp commented 1 year ago

(@thorehusfeldt I'm working on some changes locally; give me a minute to push.)

thorehusfeldt commented 1 year ago

Oh, I just did. Sorry. (Between to lectures.)

RagnarGrootKoerkamp commented 1 year ago

Pushed some more changes; please verify:

  1. Added the interaction regex to the json schema
  2. Allowing visualizers to include arguments. I simply did this as command that starts with /; not super pretty but simple.
  3. Remove {name} substitution. This isn't needed anymore now that files are always named testcase.{in,ans,..} during the generation process.
thorehusfeldt commented 1 year ago

I think there is a superfluous pair of round brackets around the {seed}y part of the regex.

thorehusfeldt commented 1 year ago

I have integrated the changes to interaction and visualizer into the schema hosted at schemastore, and my pull request was accepted a few hours ago: https://github.com/SchemaStore/schemastore/commit/488e93c6963124895ec496c4ad1d885023402626 . Please check that the language server in your editors picks this up.

  1. I added to schemastore a negative test for the interaction syntax that I find hilarious and expect at least some kind of appreciative reaction.
  2. The schema at schemastore and what we host here have diverged in various ways (the post-commit hook at schemastore reorders entries, adds whitespace, som identifiers, and is more strict.) I suggest @RagnarGrootKoerkamp copies their schema onto ours; that makes it easier to stay in sync.
RagnarGrootKoerkamp commented 12 months ago

Jup, the interaction validation works :) Sadly my editors auto-fill of the sample is not quoted but oh well. Hmm or maybe the example could contain actual newlines?

image

I'll copy the schemas back from there to here: https://github.com/RagnarGrootKoerkamp/BAPCtools/commit/f69b730c0cbeded11541d64d4f6beb2255da4881

thorehusfeldt commented 12 months ago

I don’t think “how an editor should handle newlines in the examples field of a JSON schema” is well-defined, so I fear that is tiling at windmills.