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 "bimonth" and "halfyear" options to round_date and friends #414

Merged
merged 1 commit into from
May 10, 2016
Merged

Add "bimonth" and "halfyear" options to round_date and friends #414

merged 1 commit into from
May 10, 2016

Conversation

jonboiser
Copy link
Contributor

For #268. Did a little refactoring to re-use the formula used in rounding to "quarter".

Also, deleted that parse_unit_spec function since it really was dead code, and changed an errant tab to spaces.

Still need to write specs.

@jonboiser jonboiser changed the title [WIP] add "bimonth" and "halfyear" options to round_data and friends [WIP] add "bimonth" and "halfyear" options to round_date and friends May 9, 2016
@vspinu
Copy link
Member

vspinu commented May 9, 2016

Thanks. Could you please add tests for this new feature? Also the current checks fail.

@jonboiser jonboiser changed the title [WIP] add "bimonth" and "halfyear" options to round_date and friends Add "bimonth" and "halfyear" options to round_date and friends May 10, 2016
@codecov-io
Copy link

codecov-io commented May 10, 2016

Current coverage is 77.73%

Merging #414 into master will increase coverage by +0.44%

@@             master       #414   diff @@
==========================================
  Files            42         42          
  Lines          2368       2362     -6   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           1830       1836     +6   
+ Misses          538        526    -12   
  Partials          0          0          

Powered by Codecov. Last updated by fd00b11...3a43f2d

@jonboiser
Copy link
Contributor Author

Just added tests for the new options, squashed the commits, and made sure CI passed. Let me know this needs anything else before it's ready.

@@ -1,7 +1,7 @@
#' Round, floor and ceiling methods for date-time objects.
#'
#' Users can specify whether to round to the nearest second, minute, hour, day,
#' week, month, quarter, or year.
#' week, month, "bimonth", quarter, half-year, or year.
Copy link
Member

Choose a reason for hiding this comment

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

Why is "bimonth" quoted here? half-year should probably be "halfyear".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I quoted "bimonth", because I don't think there is a proper term for "two-month period" and "bimonth" is just a made-up word. Same with "halfyear". It might be better though if the terms were consistent with the options.

@vspinu
Copy link
Member

vspinu commented May 10, 2016

Very nice!

Does this work with Date objects as expected? I know we don't many tests for that but could you add a couple of checks for Date objects as well? You can produce dates directly with ymd or make_date now.

Please also add a line to news.md with a link to #268.

This completely fixes #268, right? If so, please add [Fix #268] to the commit header to automatically close it.

Thanks a lot!

@jonboiser
Copy link
Contributor Author

jonboiser commented May 10, 2016

All done! I added the extra tests to the "floor/ceiling_date works for a variety of formats" to test that they work on Date class.

@jonboiser
Copy link
Contributor Author

Had to do a couple more pushes to get the commit message right and rebuild the docs!

@vspinu vspinu merged commit 5b8c8fe into tidyverse:master May 10, 2016
@vspinu
Copy link
Member

vspinu commented May 10, 2016

Perfect. Thanks!

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