Skip to content

nucleotide-count: Added template to stub file #565

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 3 commits into from
Jul 6, 2018

Conversation

ZapAnton
Copy link
Contributor

@ZapAnton ZapAnton commented Jul 2, 2018

Contributes to #551.

I guess it also closes #512, since no further comments were made about rewriting returned Result or using Option / panic!

Copy link
Member

@coriolinus coriolinus left a comment

Choose a reason for hiding this comment

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

Thanks @ZapAnton, this generally looks good. I've marked where I want a change, but this is nearly complete as a stub insertion.

Optionally, if you're willing, it would be great if you could also update the associated tests to ensure that the error value is the first invalid char in the input string.


pub fn count(nucleotide: char, dna: &str) -> Result<usize, ()> {
Copy link
Member

Choose a reason for hiding this comment

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

For consistency, this should return Result<usize, char>, where the error type is the invalid character.

@ZapAnton
Copy link
Contributor Author

ZapAnton commented Jul 3, 2018

Done, @coriolinus

Copy link
Member

@coriolinus coriolinus left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM!

Scheduling this one for Friday.

@coriolinus coriolinus merged commit f4c40ec into exercism:master Jul 6, 2018
@ZapAnton ZapAnton deleted the nucleotide_count_template branch July 6, 2018 08:51
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.

nucleotide-count has no template
2 participants