Skip to content

nucleotide-count: Adding a single character case#951

Merged
Insti merged 1 commit intoexercism:masterfrom
Vankog:nucleotide-count_add_single_char_case
Oct 14, 2017
Merged

nucleotide-count: Adding a single character case#951
Insti merged 1 commit intoexercism:masterfrom
Vankog:nucleotide-count_add_single_char_case

Conversation

@Vankog
Copy link
Copy Markdown
Contributor

@Vankog Vankog commented Oct 13, 2017

Splitting #895 into single PRs.

Adding a single char input case for better TDD progression as agreed upon by @petertseng (#895 (review)) and @Insti (#895 (comment))

Vankog added a commit to Vankog/problem-specifications that referenced this pull request Oct 13, 2017
Copy link
Copy Markdown
Member

@petertseng petertseng left a comment

Choose a reason for hiding this comment

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

Absolutely. I agree.

I note that if I write the commit message I must write "Add" not "Adding", but (because I can edit the commit message) that does not stop me from Approving (in the GitHub sense).

"T": 0
}
},
{
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

presently the only description that ends in punctuation, until other descriptions are changed. This means that inevitably this will cause to discuss:

  • whether the descriptions in this file should, in general, end in punctuation
  • is the (temporary, right?) inconsistency OK

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

My vote is for consistency. Punctuation should be a separate PR, or at the very least a separate commit.

Copy link
Copy Markdown
Member

@rpottsoh rpottsoh left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Copy Markdown
Contributor

@Insti Insti left a comment

Choose a reason for hiding this comment

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

Remove the period in the description for consistency as discussed above.

Other than that I think this is good. 👍

Vankog added a commit to Vankog/problem-specifications that referenced this pull request Oct 14, 2017
@Vankog Vankog force-pushed the nucleotide-count_add_single_char_case branch from 0daf900 to 236c488 Compare October 14, 2017 14:24
Vankog added a commit to Vankog/problem-specifications that referenced this pull request Oct 14, 2017
@Vankog
Copy link
Copy Markdown
Contributor Author

Vankog commented Oct 14, 2017

done and squashed while rebasing

@Vankog Vankog force-pushed the nucleotide-count_add_single_char_case branch from 236c488 to f286161 Compare October 14, 2017 14:32
@rpottsoh
Copy link
Copy Markdown
Member

I could merge this but I think it is better to let @Insti approve the changes first.

@Insti Insti merged commit 64f27d4 into exercism:master Oct 14, 2017
@Insti
Copy link
Copy Markdown
Contributor

Insti commented Oct 14, 2017

Thanks @Vankog ❤️
This was much easier to review and merge! 👍

Vankog added a commit to Vankog/problem-specifications that referenced this pull request Oct 16, 2017
Insti added a commit that referenced this pull request Oct 17, 2017
…case

nucleotide-count: fix wrong order introduced in #951
ZapAnton added a commit to ZapAnton/rust that referenced this pull request Nov 21, 2018
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.

4 participants