Skip to content

Update nucleotide_count_test.py#98

Merged
kytrinyx merged 1 commit intoexercism:masterfrom
Oniwa:master
Aug 8, 2014
Merged

Update nucleotide_count_test.py#98
kytrinyx merged 1 commit intoexercism:masterfrom
Oniwa:master

Conversation

@Oniwa
Copy link
Copy Markdown
Contributor

@Oniwa Oniwa commented Aug 7, 2014

Removed test_dna_has_no_uracil for a couple of reasons

  • The module is named dna.py and in the README.md we see that DNA does not contain uracil only RNA does
  • If 'U' is added to the dictionary it causes test_counts_all_nucleotides, test_empty_dna_string_has_no_nucleotides, and test_repetitive_sequence_has_only_guanosine to fail.
  • I believe that counting for 'U' should also throw a ValueError

As of now the only solution I could think of was to add the following:
if nucleotype is 'U': return 0
which seems inelegant to me.

Hope this helps,

Oniwa

Removed test_dna_has_no_uracil
@sjakobi
Copy link
Copy Markdown
Contributor

sjakobi commented Aug 7, 2014

I agree that this exercise is somewhat inelegant for the reasons that you give.

But removing the test case for uracil doesn't fix it IMO - now the user is left wondering how to treat uracil which gets a special mention in the README.

@kytrinyx: What's your opinion here?

@kytrinyx
Copy link
Copy Markdown
Member

kytrinyx commented Aug 7, 2014

I never really liked this exercise in any language, and I haven't found a good solution to it. I think it would be OK to just remove any mention of uracil in the README and change the exercises to just be about DNA. Anything not in DNA would then throw an error.

@Oniwa
Copy link
Copy Markdown
Contributor Author

Oniwa commented Aug 7, 2014

That makes sense to me. Especially seeing as the source for the problem(http://rosalind.info/problems/dna/) doesn't mention RNA either.

sjakobi added a commit to sjakobi/x-common that referenced this pull request Aug 7, 2014
Focus the exercise on DNA and its nucleotides A, C, G and T.
The RNA-nucleotide U(dacil) shouldn't be treated specially.

As discussed in exercism/python#98.
@kytrinyx
Copy link
Copy Markdown
Member

kytrinyx commented Aug 8, 2014

Cool, @sjakobi updated the README, so this is ready to go.

kytrinyx added a commit that referenced this pull request Aug 8, 2014
Update nucleotide_count_test.py
@kytrinyx kytrinyx merged commit ee46690 into exercism:master Aug 8, 2014
sjakobi added a commit to sjakobi/xjavascript that referenced this pull request Aug 17, 2014
sjakobi added a commit to sjakobi/xlisp that referenced this pull request Aug 17, 2014
sjakobi added a commit to sjakobi/xclojure that referenced this pull request Aug 17, 2014
sjakobi added a commit to sjakobi/xcoffeescript that referenced this pull request Aug 17, 2014
sjakobi added a commit to sjakobi/xcpp that referenced this pull request Aug 17, 2014
sjakobi added a commit to sjakobi/xerlang that referenced this pull request Aug 17, 2014
sjakobi added a commit to sjakobi/xlua that referenced this pull request Aug 17, 2014
sjakobi added a commit to sjakobi/xjava that referenced this pull request Aug 17, 2014
sjakobi added a commit to sjakobi/xcsharp that referenced this pull request Aug 17, 2014
sjakobi added a commit to sjakobi/xscala that referenced this pull request Aug 17, 2014
sjakobi added a commit to sjakobi/xclojure that referenced this pull request Aug 20, 2014
sjakobi added a commit to sjakobi/xclojure that referenced this pull request Aug 20, 2014
For the related discussion see exercism/python#98.

This also changes the namespace of the example solution from "dna" to
"nucleotide_count".
kytrinyx pushed a commit to exercism/csharp that referenced this pull request Aug 31, 2014
kytrinyx pushed a commit to exercism/erlang that referenced this pull request Aug 31, 2014
kytrinyx pushed a commit to exercism/java that referenced this pull request Aug 31, 2014
kytrinyx pushed a commit to exercism/coffeescript that referenced this pull request Aug 31, 2014
kytrinyx pushed a commit to exercism/DEPRECATED.javascript that referenced this pull request Aug 31, 2014
kytrinyx added a commit that referenced this pull request Sep 2, 2014
Update nucleotide_count_test.py
kytrinyx pushed a commit to exercism/cpp that referenced this pull request Sep 4, 2014
kytrinyx pushed a commit to exercism/lua that referenced this pull request Sep 5, 2014
kytrinyx pushed a commit to exercism/scala that referenced this pull request Oct 9, 2014
kytrinyx pushed a commit to exercism/common-lisp that referenced this pull request Oct 19, 2014
sjakobi added a commit to sjakobi/xclojure that referenced this pull request Nov 24, 2014
For the related discussion see exercism/python#98.

This also changes the namespace of the example solution from "dna" to
"nucleotide_count".
rubysolo pushed a commit to exercism/clojure that referenced this pull request Dec 1, 2014
For the related discussion see exercism/python#98.

This also changes the namespace of the example solution from "dna" to
"nucleotide_count".
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.

3 participants