Skip to content
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

Add version_restriction_test #248

Conversation

straight-shoota
Copy link
Member

Also transform helper methods to class methods for better isolation and testability.

This is a preliminary step for implementing #184

@straight-shoota straight-shoota force-pushed the jm/feature/version_restriction_test branch 2 times, most recently from e5eb0fc to a4509ea Compare January 6, 2019 19:38
@ysbaddaden
Copy link
Contributor

Adding tests is great. Breaking the includable modules for class methods is an opinionated stylistic change. Without further commits to support this change, it's hard to understand and validate it.

If there is a bug in NaturalSort, a fix would be very welcome.

Last but not least, the unit test file should symmetric to the file being tested. For example test/helpers/versions_test.cr

Also transform helper methods to class methods for better isolation and
testability.
@straight-shoota straight-shoota force-pushed the jm/feature/version_restriction_test branch from a4509ea to aadaee0 Compare January 6, 2019 20:30
@straight-shoota
Copy link
Member Author

straight-shoota commented Jan 6, 2019

It may be stylistic, but it's foremost practical as it makes these methods easier to test. I don't see why breaking the includes is a bad thing, if anything it clarifies the API.

There is a bug in resolving ~> restrictions. I won't fix NaturalSort but propose an improved algorithm instead (which doesn't have this bug).

From my perspective this PR is a simple improvement, self-contained and easy to review. It stands by itself and shouldn't depend on any planned changes.

Moved the file.

@ysbaddaden
Copy link
Contributor

Thanks for the tests. They were applied in #249.

@ysbaddaden ysbaddaden closed this Jan 8, 2019
@straight-shoota straight-shoota deleted the jm/feature/version_restriction_test branch January 8, 2019 21:14
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