Skip to content

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

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

Merged
merged 2 commits into from
Jun 28, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,5 @@ cache: yarn
script:
- bin/check_required_files_present
- sh bin/check_versions
- bin/check_optional
- yarn test
3 changes: 3 additions & 0 deletions OPTIONAL-KEYS.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
floating-point
big-integer
unicode
12 changes: 11 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ is easier to understand with an example:

```json
{ "exercise": "foobar"
, "version" : "1.0.0"
, "version" : "1.1.0"
, "comments":
[ " Comments are always optional and can be used almost anywhere. "
, " "
Expand Down Expand Up @@ -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." ]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

, "property" : "foo"
, "input" : {
"word" : "28948022309329048855892746252171976962977213799489202546401021394546514198529"
}
, "expected" : null
}
, { "description": "Bar'ing a name with numbers gives an error"
, "property" : "bar"
, "input" : {
Expand All @@ -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.
Copy link
Contributor Author

@SaschaMann SaschaMann May 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


### Test Data Versioning

Expand Down
39 changes: 39 additions & 0 deletions bin/check_optional
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
#!/bin/sh

# This script checks that 'optional' fields in canonical-data.json are listed.

optional_keys_file=OPTIONAL-KEYS.txt
allowed_optional=$(jq -nR '[inputs]' $optional_keys_file)

failed=0

check_optional_for_file() {
json_file=$1
echo "Checking 'optional' fields in $json_file"

present_optional=$(jq '[ .cases[] | recurse(.cases[]?) | .optional | select(. != null) ]' $json_file)
invalid_optional=$(jq --null-input \
--argjson allowed "$allowed_optional" \
--argjson present "$present_optional" \
--raw-output '$present - $allowed | join("\n")')

if [ ! -z "$invalid_optional" ]; then
echo "Invalid optional fields:"
echo "$invalid_optional" | perl -pe 's/^/ - /'
echo

failed=1
fi
}

for json_file in $(git diff --name-only master HEAD | grep '^exercises/.*/canonical-data\.json$'); do
check_optional_for_file $json_file
done

if [ $failed -gt 0 ]; then
echo "Allowed optional fields (see $optional_keys_file) are:"
cat $optional_keys_file | perl -pe 's/^/ - /'
exit 1
fi

exit 0
10 changes: 9 additions & 1 deletion canonical-schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
"self": { "vendor" : "io.exercism"
, "name" : "canonical-data"
, "format" : "jsonschema"
, "version": "1-0-0"
, "version": "1-1-0"
},

"$ref": "#/definitions/canonicalData",
Expand Down Expand Up @@ -86,6 +86,7 @@
, "required" : ["description", "property", "input", "expected"]
, "properties" :
{ "description": { "$ref": "#/definitions/description" }
, "optional" : { "$ref": "#/definitions/optional" }
, "comments" : { "$ref": "#/definitions/comments" }
, "property" : { "$ref": "#/definitions/property" }
, "input" : { "$ref": "#/definitions/input" }
Expand All @@ -100,6 +101,7 @@
, "required" : ["description", "cases"]
, "properties" :
{ "description": { "$ref": "#/definitions/description" }
, "optional" : { "$ref": "#/definitions/optional" }
, "comments" : { "$ref": "#/definitions/comments" }
, "cases" : { "$ref": "#/definitions/testGroup" }
}
Expand All @@ -111,6 +113,12 @@
, "type" : "string"
},

"optional":
{ "description": "An identifier for similar optional test cases (kebab-case)"
, "type" : "string"
, "pattern" : "^[a-z]+(-[a-z]+)*$"
},

"property":
{ "description": "A letters-only, lowerCamelCase property name"
, "type" : "string"
Expand Down