Skip to content

nth-prime: Add JSON test data#332

Merged
kytrinyx merged 1 commit intoexercism:masterfrom
clettenberg:nth-prime-json
Sep 19, 2016
Merged

nth-prime: Add JSON test data#332
kytrinyx merged 1 commit intoexercism:masterfrom
clettenberg:nth-prime-json

Conversation

@clettenberg
Copy link
Copy Markdown
Contributor

Adding JSON test data to go along with my xruby generator

@kotp
Copy link
Copy Markdown
Member

kotp commented Aug 11, 2016

👍 from me.

Comment thread nth-prime.json Outdated
],
"cases": [
{
"description": "first",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Consistency in descriptions: either add 'prime' here or remove it for sixth and big
My preference is add it.

@Insti
Copy link
Copy Markdown
Contributor

Insti commented Aug 11, 2016

Looks good, I only have a couple of minor nitpicks with descriptions.

@petertseng
Copy link
Copy Markdown
Member

Numbers all look correct, anyone have opinions on representing them as numbers rather than strings? Would force implementing languages to do the string -> integer parse if string, is it OK?

@Insti
Copy link
Copy Markdown
Contributor

Insti commented Aug 11, 2016

Numbers would be better.

@clettenberg
Copy link
Copy Markdown
Contributor Author

Updated the descriptions and changed the strings to numbers 👍

@Insti
Copy link
Copy Markdown
Contributor

Insti commented Aug 12, 2016

From #332 (comment)

I would simply remove the "weird case" and leave each implementation to decide how to deal with it. That would probably be better for languages that can enforce some restrictions in the type system.

This is the "weird case":

    {
       "description": "there is no zeroth prime",
       "input": 0,
       "expected": false
    }

It's better to have a test in the json that is ignored by a few tracks than not have a test-case in there that is implemented by many.
I think it's good to have some defined behaviour with regard to zero.
Language tracks can decide to not implement the test or treat the expected value as "raise an exception" as appropriate.

Does anyone else have an opinion on this?

@kytrinyx
Copy link
Copy Markdown
Member

If we make it optional, let's add a json key or something to indicate that so that generators can decide to keep or skip optional exercises.

I don't have an opinion in this specific case though. I tend to prefer to have the zeroth case defined, but I also like math, so I'm biased.

@wobh
Copy link
Copy Markdown

wobh commented Aug 13, 2016

It seems to me, at this level, we should have a field that describes valid inputs and only provide test cases of those inputs--"happy path" tests. In this case, inputs should be positive integers, language tracks should implement the failure cases. I think this would jive better with the track-dependent "difficulty" metadata that's coming up soon.

@kytrinyx
Copy link
Copy Markdown
Member

But if there are good examples of failure cases, then it would be useful to document them.

Perhaps under a key for just failure cases, and people can do with it what they please?

@ErikSchierboom
Copy link
Copy Markdown
Member

@kytrinyx I kinda like that, separating the "success" from the "failure" test cases.

@catb0t
Copy link
Copy Markdown
Contributor

catb0t commented Sep 12, 2016

Can we move this forward? It looks good to me. @kytrinyx

@kytrinyx
Copy link
Copy Markdown
Member

Yepp, this looks great. Thanks @cacqw7! (Also thanks for the ping @catb0t)

@petertseng
Copy link
Copy Markdown
Member

Ah, right, we'll need to move it from nth-prime.json to exercises/nth-prime/canonical-data.json

petertseng added a commit that referenced this pull request Sep 27, 2016
After #332 was created but before it was merged, #351 was created and
merged moving all exercises to a new structure. The nth-prime JSON file
should now be moved to its place in the new structure as well.
emcoding pushed a commit that referenced this pull request Nov 19, 2018
Add list-ops exercise.

There still may be some issues, see PR #332 for details.
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.

9 participants