[RFC] Add 'optional' key to canonical-schema #1518

SaschaMann · 2019-05-10T15:50:51Z

WAIT FOR #1496 BEFORE MERGING

This adds the most-liked implementation of an optional key. See #1492, #1496 for more info.

Ideally this should be a draft PR, but I wanted CI to run to spot potential issues.

SaschaMann · 2019-05-10T15:52:48Z

README.md

@@ -92,6 +92,15 @@ is easier to understand with an example:
              }
            , "expected"   : null
            }
+            { "description": "Foo'ing a very big number returns nothing"
+            , "optional"   : "big-ints"
+            , "comments"   : [ "Making this test case pass requires using BigInts." ]


The example in #1492 explains in the comments that "Your track may choose to skip this test". That applies to all cases with an optional-key, so it doesn't need to be added to every case, imo. The comment outlines why the test is optional.

SaschaMann · 2019-05-10T15:54:24Z

README.md

@@ -118,6 +127,7 @@ There are also some conventions that must be followed:
  - If an error is expected (because the input is invalid, or any other reason), the value at `"expected"` should be an object containing exactly one property, `"error"`, whose value is a string.
    - The string should explain why the error would occur.
    - A particular track's implementation of the exercise **need not** necessarily check that the error includes that exact string as the cause, depending on what is idiomatic in the language (it may not be idiomatic to check strings for error messages).
+  - Test cases that only some tracks should implement, for example because it would unnecessarily increase the complexity of the exercise in some but not all languages, mark it with an `optional`-key. Multiple cases related to the same reason for optionality should have the same key. The decision that a test case is optional will often be made in the PR discussion, so don't worry about it too much while creating a PR.


The decision that a test case is optional will often be made in the PR discussion, so don't worry about it too much while creating a PR.

This isn't really a convention that must be followed, but I've added it to not discourage contributors since the decision if a case only works on certain tracks isn't easy and requires input from multiple maintainers. However, I don't mind dropping it either if it doesn't fit this list.

ErikSchierboom

I like it!

ErikSchierboom · 2019-05-16T09:26:44Z

@SaschaMann I think the WIP marker can be removed so we can merge this.

rpottsoh · 2019-05-16T11:32:33Z

#1496 is still open. Opening comment to this PR says to wait for #1496 before merging. I assume that means this PR shouldn't be merged until that issue is closed.

sshine · 2019-06-26T14:31:08Z

A practical note:

We should find a place to keep track of the values we use, since it would be a pity to have int, ints, integers, float, floats, floating-points, bigints, big-int, big-ints, big-integer, big-integers, and so on. Having the schema flexible is probably nice, but we want to document the keys being used.

Perhaps even make a CI test that fails if one is used but isn't listed in this place.

SleeplessByte · 2019-06-26T15:27:29Z

I prefer a similar solution to https://github.com/exercism/problem-specifications/blob/master/TOPICS.txt, but then for these keys. It's easy to build CI around that.

sshine · 2019-06-26T16:19:23Z

WAIT FOR #1496 BEFORE MERGING

#1496 is a meta discussion mentioning 5 things:

optional flags, which this PR addresses.
immutability flags (High-Scores: add immutability test #1493) which had stalled. I've proposed that we close it for reasons.
other types of tests (Schema optimises for testing f(inputs) = output. What of other types of tests? #1225) which is a closed issue that is mentioned for inspiration.
identity field (Proposal: Add canonical data "identifier" field to separate concern from test "description" #1473) which is still in an early stage. I've proposed that we either grow on the idea or abandon it, but either way do not make this additional feature block the optional flags feature.
multi-line strings (Meta: adding new keys to the schema #1496) which has not reached a proposal yet.

I propose that we either "A":

Abandon High-Scores: add immutability test #1493 (feel free to disagree, of course)
Wait with merging Proposal: Add canonical data "identifier" field to separate concern from test "description" #1473 (early stage) and Meta: adding new keys to the schema #1496 (early stage)
Go ahead and merge this PR and start adding optional keys.

Or "B":

Abandon High-Scores: add immutability test #1493 (feel free to disagree, of course)
Set a time limit (e.g. 4-8 weeks) for growing the ideas of Proposal: Add canonical data "identifier" field to separate concern from test "description" #1473 and Meta: adding new keys to the schema #1496.
We wait with merging this PR and merge as many proposals as have reached accepted proposals in close succession.

As @SleeplessByte says in #1496:

As long as we merge them in close succession (field proposals), we reduce the burden on generator maintainers.

Forgive my ignorance, I do not understand the cost of being a test generator maintainer.

If time is what we need, open source moves very slowly anyways.

But it creates an organizational problem that we put "Depends on #1496" on a PR, when #1496 branches five-ways into directions with hours worth of reading before you know if the particular proposals have stalled because of personal indifferences, design problems, design impossibilities, lack of activity, or something else.

So now I've went through all those and posted proposals to get the current PR's feature, because that's something I find to be missing on a weekly basis.

SaschaMann · 2019-06-26T17:29:23Z

When I opened this PR and added

WAIT FOR #1496 BEFORE MERGING

there was still an on-going discussion whether or not we should add any keys. Since then we found out that an extra key doesn't break anything, so I'm in favour of merging them independantly from each other. Maintainers who prefer handling all the changes at once can wait for the other issues to be resolved.

multi-line strings (#1496) which has not reached a proposal yet.

There's a proposal in #1496 but no PR.

(After reading the latest comments in #1496 I realise this comment probably belongs there instead. It's quite chaotic indeed)

It's easy to build CI around that.

@SleeplessByte In that case, could you add it to this PR? :)

SleeplessByte · 2019-06-26T22:20:31Z

there was still an on-going discussion whether or not we should add any keys. Since then we found out that an extra key doesn't break anything, so I'm in favour of merging them independantly from each other. Maintainers who prefer handling all the changes at once can wait for the other issues to be resolved.

Yeah. This is my only concern.

I like your proposals @sshine!

bencoman · 2019-06-27T00:09:22Z

Even if its useful to group addition of the other keys together, its also useful to concentrate on a single case for the first one.

As the initiator of identity field (#1473), I'm having some progress with the alternative of slimming down the descriptions, but although the feedback is worthwhile its slow to work through. So I'm not yet ready to let go of #1473, but it may end up not needed. So I don't think #1473 should hold up this PR.

But it creates an organizational problem that we put "Depends on #1496"

If #1496 was code in a PR, you'd wait for it to be closed. As a discussion, I'd only expect a comment on #1496 that this PR will proceed as the first test case.

For me Proposal "A" is the way to go.

sshine · 2019-06-27T07:24:47Z

In that case, can we focus on preventing divergence in values?

I do find it valuable that the schema does not need to be updated every time we add another category of optional keys. We could, for example, create CI hook that gathers all existing optional keys like so:

jq '[ .cases[] | .optional | select(. != null) ]' exercises/rna-transcription/canonical-data.json
jq '[ .cases[] | .cases[] | .optional | select(. != null) ]' exercises/robot-simulator/canonical-data.json

and fails unless "new optional key: " is mentioned in the commit message?

Then we avoid typos, and we're forced to recognize when new optional keys are introduced.

ErikSchierboom · 2019-06-27T11:28:13Z

I like the proposals made by @sshine. My preference would be to use option A: merge this PR, abandon #1493, and personally I would also abandon #1473.

SaschaMann · 2019-06-27T12:04:36Z

and fails unless "new optional key: " is mentioned in the commit message?

I'd prefer having a text file with all optional keys. If a new key is in it or added to it, CI passes. Otherwise it fails. This makes it easier to look up which keys exist already. This doesn't require an update to the schema, it just requires a file like TOPICS.txt. However, I have no experience with jq or other tools that deal with JSON in bash, so I don't know if this is easily possible.

sshine · 2019-06-27T12:38:53Z

@SleeplessByte wrote:

I prefer a similar solution to TOPICS.txt

@SaschaMann wrote:

I'd prefer having a text file with all optional keys.

Then how about creating the file optional-keys.json which contains a list of strings?

[
  "floating-point",
  "big-integer",
  "unicode"
]

Or alternatively a self-documenting file:

{
  "description": "This file contains values for the 'optional' field in tests.",
  "optional": [
    "floating-point",
    "big-integer",
    "unicode"
  ]
}

sshine · 2019-06-27T12:40:51Z

I'll try to provide a CI script shortly.

SaschaMann · 2019-06-27T12:43:11Z

What's the benefit of a JSON file with just an array over a plain text file like TOPICS.txt?

sshine · 2019-06-27T12:47:06Z

A common data format. It doesn't matter much to jq, though, as it has a --raw-input option.

For parsing purposes, TOPICS.txt isn't a good example, because it contains headers.

SaschaMann · 2019-06-27T12:53:04Z

I don't think headers are necessary (for now anyway), just one key per line in a plain text fine seems fine.

It doesn't matter much to jq, though, as it has a --raw-input option.

I'd argue against JSON for this list in that case because the lack of trailing commas will result in two lines shown as changed in the diff when adding a key. This is a very minor thing obviously, but if there's no other benefit to JSON I feel like the added (tiny) annoyance isn't worth it, imo.

sshine · 2019-06-27T14:45:49Z

@SaschaMann: I believe that since this PR is based on your master, I cannot push directly to it.

So I kindly advice you to cherry-pick https://github.com/sshine/exercism-problem-specifications/commit/e0194b5074026e65391d6f509c14df8f7e2eadee so that this contribution can become part of the present PR. Edit: I've tested that it works for everyone to watch in #1541. Edit: We may additionally want to validate that additions to OPTIONAL-KEYS.txt validate the regex since JSON schema validation would have given us that.

Additionally, create a plaintext file `OPTIONAL-KEYS.txt` in the root directory containing allowed values for the `optional` field in canonical-data.json according to #1518.

SaschaMann · 2019-06-27T15:01:21Z

Strange, it's a branch on this repo, so you should be able to push to it. Anyway, I've cherry-picked the commit. Seems like a solid CI solution, thanks for writing it!

I'll remove the [WIP], since it only needs reviewing now.

sshine · 2019-06-27T15:07:32Z

Ah, shrug. There's a lot related to the GitHub workflows that I'm still getting used to.

Also, I took the liberty of naming the file OPTIONAL-KEYS.txt and pre-defining three optional keys: floating-point, big-integer, unicode in no particular order, since those are the ones I've been picking up on in the recent past. If anyone feels the file should have another name, or floating-point-number is more precise, or that unicode should be 𝕦𝕟𝕚𝕔𝕠𝕕𝕖 (OK, that's not possible), I have very few strong opinions.

SleeplessByte · 2019-06-28T00:56:34Z

These changes look great @sshine . This is exactly like I wanted/preferred ❤️

𝕦𝕟𝕚𝕔𝕠𝕕𝕖

ha

ErikSchierboom

Great work!

Add 'optional' key to canonical-schema

6fc531e

SaschaMann added the cross-track consistency label May 10, 2019

SaschaMann commented May 10, 2019

View reviewed changes

SaschaMann mentioned this pull request May 10, 2019

Proposal: Add flag to mark test cases as optional to canonical schema #1492

Closed

SaschaMann requested review from ErikSchierboom and SleeplessByte and removed request for ErikSchierboom and SleeplessByte May 10, 2019 16:01

ErikSchierboom approved these changes May 11, 2019

View reviewed changes

ErikSchierboom requested a review from petertseng May 11, 2019 11:39

petertseng removed their request for review May 11, 2019 23:13

ErikSchierboom approved these changes May 14, 2019

View reviewed changes

sshine mentioned this pull request May 14, 2019

Meta: adding new keys to the schema #1496

Open

ErikSchierboom requested a review from sshine May 14, 2019 08:14

sshine approved these changes May 15, 2019

View reviewed changes

sshine mentioned this pull request May 28, 2019

hamming: Remove empty strand errors #1522

Closed

sshine mentioned this pull request Jun 12, 2019

Pangram: Add non-basic characters in the test suite exercism/haskell#824

Closed

sshine mentioned this pull request Jun 26, 2019

triangle: actively ignore cases with float input exercism/ocaml#338

Merged

ErikSchierboom approved these changes Jun 26, 2019

View reviewed changes

Create and add bin/check_optional to .travis.yml

9e11603

Additionally, create a plaintext file `OPTIONAL-KEYS.txt` in the root directory containing allowed values for the `optional` field in canonical-data.json according to #1518.

SaschaMann requested a review from SleeplessByte June 27, 2019 15:01

SaschaMann changed the title ~~[WIP] Add 'optional' key to canonical-schema~~ [RFC] Add 'optional' key to canonical-schema Jun 27, 2019

sshine merged commit 296d1d1 into master Jun 28, 2019

sshine deleted the new-canonical-data-keys branch June 28, 2019 06:29

ErikSchierboom reviewed Jun 28, 2019

View reviewed changes

sshine mentioned this pull request Sep 26, 2019

say: Tests vulnerable to octal interpretation of chunks #1586

Closed

sshine mentioned this pull request Nov 4, 2019

Pangram tests do not match exercise specification exercism/haskell#880

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC] Add 'optional' key to canonical-schema #1518

[RFC] Add 'optional' key to canonical-schema #1518

SaschaMann commented May 10, 2019 •

edited

Loading

SaschaMann May 10, 2019

SaschaMann May 10, 2019 •

edited

Loading

ErikSchierboom left a comment

ErikSchierboom commented May 16, 2019

rpottsoh commented May 16, 2019

sshine commented Jun 26, 2019

SleeplessByte commented Jun 26, 2019 •

edited

Loading

sshine commented Jun 26, 2019 •

edited by bencoman

Loading

SaschaMann commented Jun 26, 2019 •

edited

Loading

SleeplessByte commented Jun 26, 2019

bencoman commented Jun 27, 2019 •

edited

Loading

sshine commented Jun 27, 2019

ErikSchierboom commented Jun 27, 2019

SaschaMann commented Jun 27, 2019 •

edited

Loading

sshine commented Jun 27, 2019

sshine commented Jun 27, 2019

SaschaMann commented Jun 27, 2019

sshine commented Jun 27, 2019

SaschaMann commented Jun 27, 2019 •

edited

Loading

sshine commented Jun 27, 2019 •

edited

Loading

SaschaMann commented Jun 27, 2019 •

edited

Loading

sshine commented Jun 27, 2019

SleeplessByte commented Jun 28, 2019 •

edited

Loading

ErikSchierboom left a comment

[RFC] Add 'optional' key to canonical-schema #1518

[RFC] Add 'optional' key to canonical-schema #1518

Conversation

SaschaMann commented May 10, 2019 • edited Loading

WAIT FOR #1496 BEFORE MERGING

SaschaMann May 10, 2019

Choose a reason for hiding this comment

SaschaMann May 10, 2019 • edited Loading

Choose a reason for hiding this comment

ErikSchierboom left a comment

Choose a reason for hiding this comment

ErikSchierboom commented May 16, 2019

rpottsoh commented May 16, 2019

sshine commented Jun 26, 2019

SleeplessByte commented Jun 26, 2019 • edited Loading

sshine commented Jun 26, 2019 • edited by bencoman Loading

SaschaMann commented Jun 26, 2019 • edited Loading

SleeplessByte commented Jun 26, 2019

bencoman commented Jun 27, 2019 • edited Loading

sshine commented Jun 27, 2019

ErikSchierboom commented Jun 27, 2019

SaschaMann commented Jun 27, 2019 • edited Loading

sshine commented Jun 27, 2019

sshine commented Jun 27, 2019

SaschaMann commented Jun 27, 2019

sshine commented Jun 27, 2019

SaschaMann commented Jun 27, 2019 • edited Loading

sshine commented Jun 27, 2019 • edited Loading

SaschaMann commented Jun 27, 2019 • edited Loading

sshine commented Jun 27, 2019

SleeplessByte commented Jun 28, 2019 • edited Loading

ErikSchierboom left a comment

Choose a reason for hiding this comment

SaschaMann commented May 10, 2019 •

edited

Loading

SaschaMann May 10, 2019 •

edited

Loading

SleeplessByte commented Jun 26, 2019 •

edited

Loading

sshine commented Jun 26, 2019 •

edited by bencoman

Loading

SaschaMann commented Jun 26, 2019 •

edited

Loading

bencoman commented Jun 27, 2019 •

edited

Loading

SaschaMann commented Jun 27, 2019 •

edited

Loading

SaschaMann commented Jun 27, 2019 •

edited

Loading

sshine commented Jun 27, 2019 •

edited

Loading

SaschaMann commented Jun 27, 2019 •

edited

Loading

SleeplessByte commented Jun 28, 2019 •

edited

Loading