Skip to content

custom-set: use generics and tidy up tests #957

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 18, 2017

Conversation

FridaTveit
Copy link
Contributor

Should hopefully fix custom-set part of issue #230.


Reviewer Resources:

Track Policies

@FridaTveit FridaTveit force-pushed the CustomSetUseGenerics branch from 1f1391b to 716cc18 Compare October 17, 2017 15:16
@stkent
Copy link
Contributor

stkent commented Oct 17, 2017

Looks good! I just have a couple questions:

Was there a method to deciding when a test should use int/char/String? I can't see a pattern and it's not hugely important to make sure they balance out, but I normally cycle through to make sure they distribution is roughly equal. Up to you if you'd like to do something similar.

Also, in reviewing against the canonical data, it looks like this test is currently up to spec (or almost) up to spec. If you could spend a minute double-checking my work and adding a version file in .meta, that would be awesome!

@stkent stkent self-assigned this Oct 17, 2017
@FridaTveit
Copy link
Contributor Author

As I just commented on the binary-search review, no I don't have a strict pattern for when a test should use int/char/string. Can change it to be more consistent if you think that would be useful?

And yes, will make sure the tests follow the canonical data and add a version file :)

@FridaTveit
Copy link
Contributor Author

Will change it to cycle through Character, String, Integer as it's probably best to be consistent :)

@stkent
Copy link
Contributor

stkent commented Oct 18, 2017

Looking 💯! Thanks for your hard work on this and hacktoberfest triage!

@stkent stkent merged commit 34c804b into exercism:master Oct 18, 2017
@FridaTveit FridaTveit deleted the CustomSetUseGenerics branch November 10, 2017 11:49
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.

2 participants