Skip to content

nth-prime: Add JSON test data #332

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
Sep 19, 2016
Merged

Conversation

clettenberg
Copy link
Contributor

Adding JSON test data to go along with my xruby generator

@kotp
Copy link
Member

kotp commented Aug 11, 2016

👍 from me.

],
"cases": [
{
"description": "first",
Copy link
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
Contributor

Insti commented Aug 11, 2016

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

@petertseng
Copy link
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
Contributor

Insti commented Aug 11, 2016

Numbers would be better.

@clettenberg
Copy link
Contributor Author

Updated the descriptions and changed the strings to numbers 👍

@Insti
Copy link
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
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

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
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
Member

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

@catb0t
Copy link
Contributor

catb0t commented Sep 12, 2016

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

@kytrinyx
Copy link
Member

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

@petertseng
Copy link
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