nucleotide-count: Add canonical-data.json

[bunnymatic]

Nucleotide count description: https://github.com/exercism/x-common/blob/master/exercises/nucleotide-count/description.md

[bunnymatic]

I noticed that the current Java test suite for Nucleotide includes a test that checks for exceptions being thrown. I did not include that test here but if I should, I see a pattern in here that i could use... https://github.com/exercism/x-common/blob/master/exercises/grains/canonical-data.json

[petertseng]

Thanks, these cases are looking good.

There are some cases that I would wonder about whether to add, all related to invalid inputs;

• invalid nucleotide in strand of either function
• invalid nucleotide in count

Past discussions appear to be:

• test cases for nucleotide count should include handing of invalid sequences rust#149
• nucleotide-count, invalid input cpp#50
• nucleotide-count: now both count() and nucleotide_counts() reject invalid input. python#247
• Total functions in nucleotide exercise. haskell#73
• Add exception handling for wrong nucleotides. ruby#94
• nucleotide count: adds strand validation to tests haskell#60
• Adding tests for DNA validation in Ruby. exercism#671

So since we are adding the canonical data now, now seems as good a time as any to discuss whether these cases should be included. If so, let's get them added. If not, I think we can merge what's currently here as-is, right? Maybe nucleotideCounts("GGGGGGGG") would be helpful to show that the solution isn't just special-casing the empty dna strand and can still handle characters that are absent.

[bunnymatic]

sounds good. don't merge it yet. I'll add a couple of those suggested items.

…

On Sun, Jan 22, 2017 at 6:18 PM, Peter Tseng ***@***.***> wrote: ***@***.**** commented on this pull request. Thanks, these cases are looking good. There are some cases that I would wonder about whether to add, all related to invalid inputs; - invalid nucleotide in strand of either function - invalid nucleotide in count Past discussions appear to be: - exercism/rust#149 <exercism/rust#149> - exercism/cpp#50 <exercism/cpp#50> - exercism/python#247 <exercism/python#247> - exercism/haskell#73 <exercism/haskell#73> - https://github.com/exercism/xruby/pull/94/files - https://github.com/exercism/xhaskell/pull/60/files - exercism/exercism#671 <exercism/exercism#671> So since we are adding the canonical data now, now seems as good a time as any to discuss whether these cases should be included. If so, let's get them added. If not, I think we can merge what's currently here as-is, right? Maybe nucleotideCounts("GGGGGGGG") would be helpful to show that the solution isn't just special-casing the empty dna strand and can still handle characters that are absent. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#505 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAaFdK8SUMkA3tgWI54xyF17SiK2yIhYks5rVA4MgaJpZM4Lqlft> .

[bunnymatic]

So... if you're inclined to start a pattern or continue the pattern of "an exception was thrown", i'm happy to add something like:

   {
        "description": "this whatever raises an exception",
        "strand": "bogus",
        "expected": -1
    }

as needed. Should I proceed with that? Do we need a quorum of 👍 's?

[jtigger]

While it hasn't gotten marked as "policy", there was something that looks like rough consensus with regards to exceptional values in #401:

{
  "description": "garbage input",
  "input": "100$#^0002",
  "expected": {
    "error": {
      "message": "Invalid input."
    }
}

[bunnymatic]

that seems better than -1 for sure

…

On Sun, Jan 22, 2017 at 7:53 PM, John Ryan ***@***.***> wrote: While it hasn't gotten marked as "policy", there was something that looks like rough consensus with regards to exceptional values in #401 <#401>: { "description": "garbage input", "input": "100$#^0002", "expected": { "error": { "message": "Invalid input." } } — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#505 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAaFdAJXFSkDdg_Gkwg7h6A6IRLyx4-Hks5rVCQfgaJpZM4Lqlft> .

[bunnymatic]

I've added a proposal for 2 new exception cases. One is handled (already) in the java tests for this exercise.

[jtigger]

I like the simpler syntax.

[jtigger]

Thanks for doing this, btw, @bunnymatic; every time someone codifies an exercise, an angel gets her wings. 👼

But seriously, it significantly reduces the work required to port. Even for tracks without a test generator, it's far far easier for both the contributor and the reviewer with this kind of data in hand. 🙇

[Insti]

I really don't like that this problem is trying to test 2 things.

Either it should count individual nucleotides, or count all of them.

Someone convince me that doing both is a good idea.

[petertseng]

How came it to be this way?

Looks like through exercism/exercism@4c43852

Where'd the problem come from? http://rosalind.info/problems/dna/ - there, it actually just wants the four counts at once, never asking for the individuals. So asking for the individuals is our extension!

Should it stay this way?

I have no arguments to offer for either side.

I note that if going back to taking only all counts, this problem becomes quite similar to word-count. Compared to word count, there is no longer the obligation to split on words, but an added obligation to validate the input for valid nucleotides only.

[petertseng]

validates nucleotides in requested nucleotide

validates nucleotides in the strand

Shall there also be a test for validating nucleotides in the strand for count, as well?

[Insti]

Counting them all at once seems like it makes for a more interesting problem with a couple of different approaches to discuss.

My vote is for only test for the case where all nucleotides are counted at once.

[bunnymatic]

@petertseng - i'm down with another exception test around validate nucleotides during count. I'll wait until I know whether or not we're going to do the single "countAll" tests or both.

[bunnymatic]

As for the two test discussion, I've seen (in reviewing this for other folks on the site) different approaches that may be a remnant of the problem statement and the test cases. I've seen folks solve it by writing count(nucleotide) = function() {} where that runs the strand and pulls out only that nucleotide and counts it up. then counts() simply runs through all known nucleotides and calls count(nuc). The other approach has been - counts() runs the whole strand and counts everything and stashes it on the instance. Then count just looks into this instance map. These two solutions are good for discussion points around memoization/caching, lookup speed etc.

If we went with the "count all" as the only tests, do you think we'd still get both of these solutions and would it reduce the possibility for discussion/learning around these exercises? food for thought.

[Insti]

'one repeated nucleotide'

[Insti]

If we went with the "count all" as the only tests, do you think we'd still get both of these solutions and would it reduce the possibility for discussion/learning around these exercises?

It would not reduce the discussion possibility.
Both approaches are still applicable for 'count all'

[Insti]

"repeated"

[bunnymatic]

Given the big rewrite, this last commit is a squish leaving only the latest version of the data file around.

[Insti]

Looks good.

[bunnymatic]

Once this is merged, we could also close this https://github.com/exercism/todo/issues/116

[petertseng]

An idea is to include a line of the form "Closes https://github.com/exercism/todo/issues/116" or "Closes https://github.com/exercism/todo/issues/116" in the commit message - then when we merge the commit, it will automatically close the issue due to https://help.github.com/articles/closing-issues-via-commit-messages/

I'm OK with these changes. It is a departure from what's currently in tracks, but anyone who wanted to argue for keeping both kinds of tests has had many days to voice their opinion.

So if nobody says anything at 24 hours since the last change, let's merge.

[petertseng]

Thanks!