Skip to content

DO NOT MERGE: trial new schema w/ input #998

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

Closed
wants to merge 1 commit into from
Closed

DO NOT MERGE: trial new schema w/ input #998

wants to merge 1 commit into from

Conversation

petertseng
Copy link
Member

No description provided.

@petertseng
Copy link
Member Author

exercises/acronym/canonical-data.json valid

what we wanted to see

@petertseng
Copy link
Member Author

petertseng commented Nov 8, 2017

fun fact: the one other exercise that is valid under the new schema, with no additional changes?

ETL.

, "property" : { "$ref": "#/definitions/property" }
, "input" : { "$ref": "#/definitions/input" }
, "expected" : { "$ref": "#/definitions/expected" }
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this disallow additional properties?

Copy link
Member Author

Choose a reason for hiding this comment

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

I currently can't think of a need for other properties. So, great question and/or idea, let's do it.

@petertseng
Copy link
Member Author

petertseng commented Nov 17, 2017

To make this transition, it is:

  • Helpful if we can do it an exercise at a time rather than all at once, to respect the limited time individuals may have.
  • Prevents additional necessary fixups if we can leverage Travis to validate that files attempted to be converted to the new schema have indeed successfully be done so.
  • Helpful for backwards compatibility that JSON files that have not been converted are still validated under the old schema, so that we can validate and support changes made to a file that has not yet been converted.
  • Helpful if we are able to track progress in some way.

I wonder if I should follow this plan:

For each exercise: Its canonical-data.json file will be verified using the new schema, unless the directory contains a file named verify-with-old-schema, in which case the file will be verified with the old schema. Failure to validate under the schema selected by this rule will cause CI to fail. We initially populate every directory that contains a canonical-data.json file with a file verify-with-old-schema. When converting a file to the new schema, simply remove the file.

@Insti
Copy link
Contributor

Insti commented Nov 17, 2017

Do we need the extra file?
Can we not just attempt to validate with the new schema and if that fails try the old schema?

@NobbZ
Copy link
Member

NobbZ commented Nov 17, 2017 via email

@petertseng
Copy link
Member Author

I currently count five exercises compliant.

@petertseng
Copy link
Member Author

currently

exercises/acronym/canonical-data.json valid
exercises/all-your-base/canonical-data.json valid
exercises/allergies/canonical-data.json valid
exercises/anagram/canonical-data.json valid
exercises/armstrong-numbers/canonical-data.json valid
exercises/beer-song/canonical-data.json valid
exercises/binary-search/canonical-data.json valid
exercises/binary/canonical-data.json valid
exercises/bob/canonical-data.json valid
exercises/book-store/canonical-data.json valid
exercises/etl/canonical-data.json valid
exercises/isbn-verifier/canonical-data.json valid
exercises/luhn/canonical-data.json valid
exercises/transpose/canonical-data.json valid

@ErikSchierboom
Copy link
Member

@petertseng Could you re-run the tool to see which exercises are now compliant? Is it also possible to see which exercise are not compliant?

@petertseng
Copy link
Member Author

Could you re-run the tool to see which exercises are now compliant?

OK, rebased on master. Since I won't be around to report when it finishes, I'll have to leave it to you to read the output when it finishes.

Is it also possible to see which exercise are not compliant?

Yes.

@ErikSchierboom
Copy link
Member

Thanks @petertseng!

The current status is as follows.

Valid:

  • exercises/acronym/canonical-data.json
  • exercises/allergies/canonical-data.json
  • exercises/all-your-base/canonical-data.json
  • exercises/anagram/canonical-data.json
  • exercises/armstrong-numbers/canonical-data.json
  • exercises/atbash-cipher/canonical-data.json
  • exercises/beer-song/canonical-data.json
  • exercises/binary/canonical-data.json
  • exercises/binary-search/canonical-data.json
  • exercises/bob/canonical-data.json
  • exercises/book-store/canonical-data.json
  • exercises/bracket-push/canonical-data.json
  • exercises/change/canonical-data.json
  • exercises/collatz-conjecture/canonical-data.json
  • exercises/connect/canonical-data.json
  • exercises/crypto-square/canonical-data.json
  • exercises/diamond/canonical-data.json
  • exercises/difference-of-squares/canonical-data.json
  • exercises/dominoes/canonical-data.json
  • exercises/etl/canonical-data.json
  • exercises/flatten-array/canonical-data.json
  • exercises/isbn-verifier/canonical-data.json
  • exercises/luhn/canonical-data.json
  • exercises/meetup/canonical-data.json
  • exercises/transpose/canonical-data.json

Invalid:

  • exercises/alphametics/canonical-data.json
  • exercises/bowling/canonical-data.json
  • exercises/circular-buffer/canonical-data.json
  • exercises/clock/canonical-data.json
  • exercises/complex-numbers/canonical-data.json
  • exercises/custom-set/canonical-data.json
  • exercises/food-chain/canonical-data.json
  • exercises/forth/canonical-data.json
  • exercises/gigasecond/canonical-data.json
  • exercises/grains/canonical-data.json
  • exercises/grep/canonical-data.json
  • exercises/hamming/canonical-data.json
  • exercises/hello-world/canonical-data.json
  • exercises/house/canonical-data.json
  • exercises/isogram/canonical-data.json
  • exercises/kindergarten-garden/canonical-data.json
  • exercises/largest-series-product/canonical-data.json
  • exercises/leap/canonical-data.json
  • exercises/list-ops/canonical-data.json
  • exercises/markdown/canonical-data.json
  • exercises/minesweeper/canonical-data.json
  • exercises/nth-prime/canonical-data.json
  • exercises/nucleotide-count/canonical-data.json
  • exercises/ocr-numbers/canonical-data.json
  • exercises/palindrome-products/canonical-data.json
  • exercises/pangram/canonical-data.json
  • exercises/pascals-triangle/canonical-data.json
  • exercises/perfect-numbers/canonical-data.json
  • exercises/phone-number/canonical-data.json
  • exercises/pig-latin/canonical-data.json
  • exercises/poker/canonical-data.json
  • exercises/pov/canonical-data.json
  • exercises/prime-factors/canonical-data.json
  • exercises/protein-translation/canonical-data.json
  • exercises/proverb/canonical-data.json
  • exercises/queen-attack/canonical-data.json
  • exercises/rail-fence-cipher/canonical-data.json
  • exercises/raindrops/canonical-data.json
  • exercises/react/canonical-data.json
  • exercises/rectangles/canonical-data.json
  • exercises/reverse-string/canonical-data.json
  • exercises/rna-transcription/canonical-data.json
  • exercises/robot-simulator/canonical-data.json
  • exercises/roman-numerals/canonical-data.json
  • exercises/rotational-cipher/canonical-data.json
  • exercises/run-length-encoding/canonical-data.json
  • exercises/saddle-points/canonical-data.json
  • exercises/say/canonical-data.json
  • exercises/scrabble-score/canonical-data.json
  • exercises/secret-handshake/canonical-data.json
  • exercises/sieve/canonical-data.json
  • exercises/space-age/canonical-data.json
  • exercises/spiral-matrix/canonical-data.json
  • exercises/sublist/canonical-data.json
  • exercises/sum-of-multiples/canonical-data.json
  • exercises/tournament/canonical-data.json
  • exercises/triangle/canonical-data.json
  • exercises/trinary/canonical-data.json
  • exercises/twelve-days/canonical-data.json
  • exercises/two-bucket/canonical-data.json
  • exercises/two-fer/canonical-data.json
  • exercises/variable-length-quantity/canonical-data.json
  • exercises/word-count/canonical-data.json
  • exercises/word-search/canonical-data.json
  • exercises/wordy/canonical-data.json
  • exercises/zebra-puzzle/canonical-data.json
  • exercises/zipper/canonical-data.json

@petertseng
Copy link
Member Author

petertseng commented Jan 5, 2018

For each exercise: Its canonical-data.json file will be verified using the new schema, unless the directory contains a file named verify-with-old-schema, in which case the file will be verified with the old schema. Failure to validate under the schema selected by this rule will cause CI to fail. We initially populate every directory that contains a canonical-data.json file with a file verify-with-old-schema. When converting a file to the new schema, simply remove the file.

Do we need the extra file?
Can we not just attempt to validate with the new schema and if that fails try the old schema?

If we do this:

  • The advantage is that we don't have that extra file to do with (we might forget to remove it, etc.). This advantage makes it easier for those who would implement the new schema (convert existing files to use it).
  • The disadvantage is that if I attempt to convert a file to the new schema but make a subtle mistake that thus makes it invalid, I can only discover the mistake by reading the CI output carefully (I must travel to travis-ci.org to do this), rather than relying on the pass/fail result of CI (which is visible right on pull request page). This disadvantage disfavours those who review PRs that purport to convert files to the new schema.

Since I predict I as a reviewer may make a mistake in reading the CI output carefully, I preferred to make the intention explicit instead of implicit.

Since to me it makes a clearer history if we avoid having commits that look like "convert to new schema" (PR gets merged) "oops, actually convert to new schema correctly this time", I am biased toward making the review as error-free as possible, despite that we burden implementors with having to remember to git rm.

However, since to date I have neither been an implementor nor a reviewer of PRs converting to the new schema, I think I had better defer my decision to those who have been in such positions.

@ErikSchierboom
Copy link
Member

What should we do with test cases where there is no input? An example of this is the hello-world exercise.

@petertseng
Copy link
Member Author

petertseng commented Jan 8, 2018

Among the choices I thought of were:

  • "input": {}
  • "input": null
  • Don't have input in the test case object at all
  • your suggestion goes here

I think I would prefer "input": {}, the cause being that:

  • It will allow us to continue saying that "input" is required in the schema and it corresponds to a value of type object, helping validate that all other exercises do have inputs.
  • It will help us confirm that we did in fact explicitly consider that each test case has no inputs, instead of having forgotten to convert it to the new schema

@rpottsoh
Copy link
Member

rpottsoh commented Jan 8, 2018

I think I would prefer "input": {}

👍

@ErikSchierboom
Copy link
Member

I think I would prefer "input": {}

Agreed!

This schema was proposed and accepted in
#996
@petertseng
Copy link
Member Author

This PR has been superseded by #1072 and #1074. It does not make sense to continue to maintain this PR.

@petertseng petertseng closed this Jan 15, 2018
@petertseng petertseng deleted the schema-with-input branch January 15, 2018 22:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants