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

Reorganize Time specs #6574

Merged

Conversation

straight-shoota
Copy link
Member

With over 1000 LOC that have evolved a lot over time, time_spec has grown to be hard to manage and quite confusing in some parts.

This PR restructures some parts of it to make it easier to maintain and follow the purpose of each test. There have been no changes to the contents of the specs.

  • Related specs are grouped together and consistently named after the methods they describe.
  • All specs for parsing and formatting (well over 400 LOC and still incomplete) are extracted to time/format_spec.cr.
  • Unnecessary helper methods are removed.
  • Refactored expectations to use plain constructors instead of parsing a string representation.
  • Refactored expectations to compare to an entire instance instead of comparing individual fields.

There are a lot of changes only affecting white space, so the diff is best viewed with adding ?w=1 (-2 CLI flag).

@straight-shoota straight-shoota force-pushed the jm/feature/time-spec-reorganize branch from 183ea61 to cb72387 Compare August 20, 2018 16:45
@straight-shoota
Copy link
Member Author

Could this get a second review and merge? It's mostly cosmetical and I'm working on a PR that's based upon this.

Copy link
Member

@sdogruyol sdogruyol left a comment

Choose a reason for hiding this comment

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

Thank you @straight-shoota 👍

@sdogruyol sdogruyol merged commit 76080b4 into crystal-lang:master Aug 28, 2018
@sdogruyol sdogruyol added this to the 0.27.0 milestone Aug 28, 2018
@straight-shoota straight-shoota deleted the jm/feature/time-spec-reorganize branch September 3, 2018 18:32
ezrast pushed a commit to ezrast/crystal that referenced this pull request Oct 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants