Skip to content

say: adding canonical-data.json #399

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 1 commit into from
Oct 13, 2016
Merged

say: adding canonical-data.json #399

merged 1 commit into from
Oct 13, 2016

Conversation

junedev
Copy link
Member

@junedev junedev commented Oct 8, 2016

This is to close https://github.com/exercism/todo/issues/142.
The test cases were quite homogeneous in the different tracks. I ignored the following outliers:

  • A couple of tracks added one or two more numbers, in OCaml there a lot more numbers (especially all the "teeens" are tested), Ruby adds a couple more big numbers. I don't think the additions add much value for the exercise.
  • The test for the upper bound is missing in Haskell and Go, Go instead let's people solve the exercise the max_int value. I prefer to have the upper bound in place (and covered by a test) as in the description of the exercise.

For discussion: The description in my PR and in the existing tests currently only contains the number that is tested instead of a real description. This is not "best practice" but changing to some declarative description would add little value and mean a lot of work for the individual tracks to adapt. Any thoughts on that?

Complete list of links to the implementations:
https://github.com/exercism/xcsharp/tree/master/exercises/say
https://github.com/exercism/xcpp/tree/master/say
https://github.com/exercism/xecmascript/tree/master/exercises/say
https://github.com/exercism/xelm/tree/master/exercises/say
https://github.com/exercism/xfsharp/tree/master/exercises/say
https://github.com/exercism/xgo/tree/master/exercises/say
https://github.com/exercism/xhaskell/tree/master/exercises/say
https://github.com/exercism/xjavascript/tree/master/exercises/say
https://github.com/exercism/xocaml/tree/master/exercises/say
https://github.com/exercism/xperl5/tree/master/say
https://github.com/exercism/xpython/tree/master/exercises/say
https://github.com/exercism/xracket/tree/master/exercises/say
https://github.com/exercism/xruby/tree/master/exercises/say
https://github.com/exercism/xscala/tree/master/exercises/say

@IanWhitney
Copy link
Contributor

Awesome! Thanks so much for this work. I think this can go in as-is, but I do have some suggestions that we may want to discuss.

I've mostly seen exception expectations written as -1, not null. But I don't think there's a hard-and-fast rule about that.

Is there a plan behind the order of the tests? There are two things I look for when ordering tests:

  • Each test introduces one new behavior (if possible)
  • Multiple tests aren't passed by making a single change

For example, these two tests probably pass with the same change

{
            "description": "one million",
            "input": 1000000,
            "expected": "one million"
        },
        {
            "description": "one million two",
            "input": 1000002,
            "expected": "one million two"
        },

And there may be some other cases. In cases like this we can usually remove one of these tests as duplicative.

I'm fine with your test descriptions.

@junedev
Copy link
Member Author

junedev commented Oct 9, 2016

To check what to do in case of exception I looked at some of the other json files and happened to see only those like binary and alphametics that use null. I switched to -1 now to follow the instructions in the readme and the majority of the other test data files.

I also removed the redundant test case "one million two". Other than that I find it a bit difficult to judge what the perfect order and minimal set would be as it might depend on the implementation path people are taking.

@Insti
Copy link
Contributor

Insti commented Oct 9, 2016

I prefer null as an indicator of exceptionality.
(but that's probably a discussion that needs to be had somewhere else. According to the current instructions -1 is apparently ok.)
Edit: Discussion about exceptional return value: #401

@petertseng
Copy link
Member

The description in my PR and in the existing tests currently only contains the number that is tested instead of a real description

I agree that it's unfortunate that some descriptions simply duplicate the expected output. What is the alternative, as it relates to this JSON file? Simply delete the description for those cases from the JSON file? I would find that acceptable, I think. I would indeed leave the description on the bounds checks though. If it's better to be consistent and have descriptions on all the cases even if some descriptions are useless, I at least find that more acceptable than having no descriptions since at least the last few cases will benefit from it.

I prefer null as an indicator of exceptionality.

I used null in JSON files that I contributed to this track, but now I have found that I was in violation of the README (and yet, nobody had complained to me!!!)

I do not consider this issue to block this PR. We will talk about it in #401.

@kytrinyx
Copy link
Member

If it's better to be consistent and have descriptions on all the cases even if some descriptions are useless, I at least find that more acceptable than having no descriptions since at least the last few cases will benefit from it.

I think I prefer consistency. Either way, let's ship this!

Thank you ❤️ ❤️

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