Please review: Implemented exercise nucleotide-count by Vankog · Pull Request #349 · exercism/javascript

Vankog · 2017-09-03T22:42:11Z

WIP
edit: done(?)

Vankog · 2017-09-04T02:20:31Z

Well it seems I'm done. Please review!

Vankog

see file comments

Vankog · 2017-09-04T02:23:52Z

@@ -1,1052 +1,1065 @@
 {


Ran the configlet fmt as described in the documentation:
https://github.com/exercism/docs/blob/master/you-can-help/implement-an-exercise-from-specification.md#configuring-the-exercise
It seems the configlet wasn't run in quite a time. Or the formatter changed drastically... don't know right now.
edit: oh it seems the fmt part is new for 12 days... so no wonder why it was never run.

Hmm. The configlet does many changes to this file, and they seem out of the scope of this MR.

We should handle configlet changes in a new PR (it'll perfectly fine if you run it and create that PR).

So, I'd leave this file untouched, just modifications related to nucleotide-count. You can do those changes compatible with the new configlet. It'll be ok. But, I wouldn't change the rest of exercises configuration in the PR.

What do you think?

Yeah, actually I did so in the JS track with a separate PR that got merged just recently. But this epiphany struck me too late for thes ecma track. ;-)
I'll split it.

Vankog · 2017-09-04T02:26:40Z

+      ],
+      "uuid": "62d60b42-93bc-4de9-90d1-1ca18a847812"
    },
    {


I put it here, because it is related to the rna-transcriptor. That's why I let it unlock with that exercise.
The uuid I generated with https://www.uuidgenerator.net

Vankog · 2017-09-04T02:27:43Z

@@ -0,0 +1,69 @@
+{


I updated and customized the package.json

package.json is kept in sync with the one in the root folder. All exercises have the same package.json. If this is different, well lose your customizations. Is there anything special (different from the main one) you need?

I see, in that case I'll revert it. It just includes some new versions and a customized description and authoring.

Vankog · 2017-09-04T02:28:33Z

+    expect(new Dna(strand).count('Z')).toEqual(0);
+    // special-character-nucleotides
+    expect(new Dna(strand).count('Ä')).toEqual(0);
+    expect(new Dna(strand).count('×')).toEqual(0);


inserting a Unicode character by design.
.... and for the lols...

Vankog · 2017-09-04T02:30:28Z

@@ -1,35 +1,35 @@
 {


formatted by configlet fmt.
Same reasoning as above.
edit: oh it seems the fmt part is new for 12 days... so no wonder why it was never run.

It could be included in the new PR with configlet changes.

tejasbubane · 2017-09-06T17:18:35Z

Hi @Vankog. Thanks a lot for your contribution! 👍

Could you please sync up the tests with the canonical tests here?
These are the common set of tests we prefer to adhere to with all languages in exercism.

rchavarria

I agree with @tejasbubane. Could you make tests fit the canonical ones?

Anyway, it's fine if you want to add more tests than the provided in the canonical ones. But they should be related to the uniqueness of JavaScript as a language.

On the other hand, I find the use of the 💩 really fun. I'd keep a expect or a test for that 😆

rchavarria · 2017-09-07T09:46:33Z

@@ -1,1052 +1,1065 @@
 {


Hmm. The configlet does many changes to this file, and they seem out of the scope of this MR.

We should handle configlet changes in a new PR (it'll perfectly fine if you run it and create that PR).

So, I'd leave this file untouched, just modifications related to nucleotide-count. You can do those changes compatible with the new configlet. It'll be ok. But, I wouldn't change the rest of exercises configuration in the PR.

What do you think?

rchavarria · 2017-09-07T09:48:26Z

@@ -1,35 +1,35 @@
 {


It could be included in the new PR with configlet changes.

rchavarria · 2017-09-07T09:52:04Z

@@ -0,0 +1,69 @@
+{


package.json is kept in sync with the one in the root folder. All exercises have the same package.json. If this is different, well lose your customizations. Is there anything special (different from the main one) you need?

Vankog · 2017-09-08T12:40:45Z

Alright, thanks for your feedback. Added some replies to the reviews.
I agree with splitting the configlet fmt into another PR. I'll do so.

About the tests: In the JS track I just got the same advice about the canonical versions.
I'll probably will open a discussion in the canonical track to discuss some changes, because I think some canonical test cases in the different exercises might be bit suboptimal. Firstly, they should provide an optimal B/TDD experience to teach about. It's a learning platform after all. Secondly, some do not seem to cover all suitable edge cases properly, but we have to look closer to the specifics to judge.

tejasbubane · 2017-09-08T13:27:37Z

About the tests: In the JS track I just got the same advice about the canonical versions.
I'll probably will open a discussion in the canonical track to discuss some changes, because I think some canonical test cases in the different exercises might be bit suboptimal. Firstly, they should provide an optimal B/TDD experience to teach about. It's a learning platform after all. Secondly, some do not seem to cover all suitable edge cases properly, but we have to look closer to the specifics to judge.

Yes, even I felt the same while looking at the tests for this exercise. Please open a discussion there.

Vankog · 2017-09-08T15:48:06Z

+      ],
+      "unlocked_by": "rna-transcription",
+      "uuid": "9debee8e-003d-4eb2-bf97-6d3764d024ca"
+    },


I put it here, because it is related to the rna-transcriptor. That's why I let it unlock with that exercise.
The uuid I generated with https://www.uuidgenerator.net

Vankog · 2017-09-08T18:10:12Z

OK, reverted the configlet formatting for this PR and removed the package.json.

I also created a discussion PR for the canonical test:
exercism/problem-specifications#895
I have yet to adjust my test accordingly and I'll wait for what it brings. Till then this PR is on hold.

tejasbubane · 2017-10-30T08:33:38Z

package.json should be added for this exercise.

I know this PR is on hold, but something I just noticed and commenting so it does not miss out later.

matthewmorgan · 2017-11-26T14:48:33Z

@Vankog what is the status of this PR? It looks like your PR to problem-specifications has been closed? Should we close this too, or are you intending to finish it?

Thanks!

Vankog · 2017-12-06T23:14:05Z

no its just closed, because it was splitted.

see exercism/problem-specifications#895 (comment)

However, until next year I am not able to work on this. Hot phase for my masters thesis... ;-)

I intend to return to these issues.

matthewmorgan · 2017-12-07T11:59:30Z

OK, we're going to close this PR for now. Thanks for the update.

Vankog changed the title ~~Implement exercise nucleotide-count~~ Please review: Implemented exercise nucleotide-count Sep 4, 2017

Vankog commented Sep 4, 2017

View reviewed changes

Vankog force-pushed the add-nucleotide-count branch from b2d920a to e9e9679 Compare September 4, 2017 02:32

rchavarria suggested changes Sep 7, 2017

View reviewed changes

Vankog force-pushed the add-nucleotide-count branch 2 times, most recently from ce53ae3 to 0ad06ed Compare September 8, 2017 15:45

Vankog commented Sep 8, 2017

View reviewed changes

Vankog force-pushed the add-nucleotide-count branch 2 times, most recently from d3d5218 to 8ecd75f Compare September 17, 2017 16:55

implemented nucleotide-count exercise

b8c2289

Vankog force-pushed the add-nucleotide-count branch from 8ecd75f to b8c2289 Compare September 17, 2017 16:56

matthewmorgan mentioned this pull request Oct 22, 2017

Port nucleotide-count exercise from JavaScript track #361

Closed

matthewmorgan closed this Dec 7, 2017

Uh oh!

Conversation

Vankog commented Sep 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Vankog commented Sep 4, 2017

Uh oh!

Vankog left a comment

Choose a reason for hiding this comment

Uh oh!

Vankog Sep 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Vankog Sep 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Vankog Sep 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Vankog Sep 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tejasbubane commented Sep 6, 2017

Uh oh!

rchavarria left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Vankog commented Sep 8, 2017

Uh oh!

tejasbubane commented Sep 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Vankog commented Sep 8, 2017

Uh oh!

tejasbubane commented Oct 30, 2017

Uh oh!

matthewmorgan commented Nov 26, 2017

Uh oh!

Vankog commented Dec 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

matthewmorgan commented Dec 7, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Vankog commented Sep 3, 2017 •

edited

Loading

Vankog Sep 4, 2017 •

edited

Loading

Vankog Sep 8, 2017 •

edited

Loading

Vankog Sep 4, 2017 •

edited

Loading

Vankog Sep 4, 2017 •

edited

Loading

Vankog commented Dec 6, 2017 •

edited

Loading