Skip to content

Add Nucleotide Count Test Generator #416 #456

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
Oct 4, 2017

Conversation

hymccord
Copy link
Contributor

@hymccord hymccord commented Oct 4, 2017

This was/is a good learning experience. I look forward to feedback and doing more work.
I tried to become as familiar as possible with the code base to come up with an elegant solution, though I didn't see a way to easily test instance properties.

As explained further in my commit message, I took the liberty to remove the Count method from the exercise. It was previously needed in he the old unit testing but with the canonical data based unit test, it was pruned.

This commit also renames the nucleotide exercise class to emulate the
rest of the exercises. I also decided to remove the count method as the
canonical data doesn't reference that this method should be tested.
@jpreese
Copy link
Contributor

jpreese commented Oct 4, 2017

Awesome @inkahootz! We always welcome the help 👍

As a heads up, if you haven't figured out already, the broken build is due to the fact that the class name was changed and the Example.cs was not changed as well. That should get Travis to go green for you.

You can also run powershell .\build.ps1 (for windows) or build.sh (for linux) to test your build before pushing to your branch if you so desire.

@hymccord
Copy link
Contributor Author

hymccord commented Oct 4, 2017

Ah, I forgot Example.cs. Doesn't show up in VS. I'll get that fixed right quick.

Copy link
Member

@ErikSchierboom ErikSchierboom left a comment

Choose a reason for hiding this comment

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

I have just one minor request (see comment), but other than that it looks fantastic!


namespace Generators.Exercises
{
class NucleotideCount : Exercise
Copy link
Member

Choose a reason for hiding this comment

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

Could you add the public modifier (for consistency)?

@hymccord
Copy link
Contributor Author

hymccord commented Oct 4, 2017

Thanks! I'll start looking for my next issue.
One question. Is there no way to test instance/static properties on a class without writing a custom template?

@ErikSchierboom
Copy link
Member

@inkahootz There should be. I'll look into it later. I'll merge when the CI is green.

@ErikSchierboom ErikSchierboom merged commit e85c9dc into exercism:master Oct 4, 2017
@ErikSchierboom
Copy link
Member

Merged! Thanks a lot @inkahootz! 🎉

@hymccord hymccord deleted the nucleotide-count-generator branch October 7, 2017 17:02
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