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

fixes #37 #41

Merged
merged 4 commits into from
Sep 15, 2017
Merged

fixes #37 #41

merged 4 commits into from
Sep 15, 2017

Conversation

qgeissmann
Copy link
Contributor

We can now use c(hms(1), hms(2)) and get a hms object back.
c(hms(1), NA) will not work because it is not implemented is base's difftime. So relevant unittest commented out for now. Should be another issue.

@codecov
Copy link

codecov bot commented Jul 25, 2017

Codecov Report

Merging #41 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #41      +/-   ##
==========================================
+ Coverage   98.63%   98.64%   +0.01%     
==========================================
  Files           6        6              
  Lines          73       74       +1     
==========================================
+ Hits           72       73       +1     
  Misses          1        1
Impacted Files Coverage Δ
R/hms.R 97.36% <100%> (+0.07%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8f4eaa3...e336389. Read the comment docs.

Copy link
Member

@krlmlr krlmlr left a comment

Choose a reason for hiding this comment

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

Thanks, looks good.

# this does not work in base R: `c(as.difftime("20:00:00"), NA)`
# so not sure whether we want it to work in hms...
# test_that("composition with NA works", {
# expect_identical(c(hms(1),NA), hms(c(1, NA)))
Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with just having expect_error() here.

context("combine")

test_that("combination keeps class and order", {
expect_identical(c(hms(1),hms(2)), hms(1:2))
Copy link
Member

Choose a reason for hiding this comment

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

Can you please add space after the commas?

Copy link
Member

@krlmlr krlmlr left a comment

Choose a reason for hiding this comment

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

Thanks, the auto-coercion from character skipped my attention before.


test_that("combination coerces to hms", {
expect_identical(c(hms(1), 2), hms(1:2))
expect_identical(c(hms(1), "00:00:02"), hms(1:2))
Copy link
Member

Choose a reason for hiding this comment

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

I'd expect this to throw an error. Base R converts to NA and warns, but we can be stricter here (and relax later if necessary):

class(as.POSIXct("2017-12-12 12:34:56 CEST"))
#> [1] "POSIXct" "POSIXt"
c(Sys.time(), "2017-12-12 12:34:56 CEST")
#> Warning in as.POSIXlt.POSIXct(x, tz): NAs introduced by coercion
#> [1] "2017-08-03 09:32:19 CEST" NA

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The behaviour for difftime is different though:

c(as.difftime("20:00:00", units="sec"), "00:00:10")
#> Time differences in secs
#> [1] 72000    10

I think we want to use as.hms() inside c(), but as.hms() will converts strings as well.
If we coerce to numeric, e.g.:

c.hms <- function(x, ...) {
  hms(c(as.numeric(x), as.numeric(...)))
}

Thoughts?
We risk having inconsistent behaviours:

d <- as.difftime("20:00:00")
c(hms(1), d)
#> 00:00:01
#> 00:00:20

Copy link
Member

Choose a reason for hiding this comment

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

Thanks. I wasn't aware of the difftime behavior.

@krlmlr
Copy link
Member

krlmlr commented Aug 31, 2017

Are you still planning to expand on this?

@qgeissmann
Copy link
Contributor Author

Yes, sorry, was quite busy in the last few weeks. I will finish this next week if it is ok.

@krlmlr
Copy link
Member

krlmlr commented Aug 31, 2017

No worries. We should keep #42 in mind, too.

@krlmlr krlmlr merged commit cf6be63 into tidyverse:master Sep 15, 2017
@krlmlr
Copy link
Member

krlmlr commented Sep 15, 2017

Thanks!

krlmlr added a commit that referenced this pull request Nov 16, 2017
- `hms()` now works correctly if all four components (days, hours, minutes, seconds) are passed (#49).
- Values with durations of over 10000 hours are now printed correctly (#48).
- `c()` now returns a hms (#41, @qgeissmann).
- Pillar support (#43).
krlmlr added a commit that referenced this pull request Nov 23, 2017
Breaking changes
----------------

- `as.hms.POSIXt()` now defaults to the current time zone, the previous default was `"UTC"` and can be restored by calling `pkgconfig::set_config("hms::default_tz", "UTC")`.

New features
------------

- Pillar support, will display `hms` columns in tibbles in color on terminals
  that support it (#43).
- New `round_hms()` and `trunc_hms()` for rounding or truncating to a given multiple of seconds (#31).
- New `parse_hms()` and `parse_hm()` to parse strings in "HH:MM:SS" and "HH:MM" formats (#30).
- `as.hms.POSIXt()` gains `tz` argument, default `"UTC"` (#28).
- `as.hms.character()` and `parse_hms()` accept fractional seconds (#33).

Bug fixes
---------

- `hms()` now works correctly if all four components (days, hours, minutes, seconds) are passed (#49).
- `hms()` creates a zero-length object of class `hms` that prints as `"hms()"`.
- `hms(integer())` and `as.hms(integer())` both work and are identical to `hms()`.
- Values with durations of over 10000 hours are now printed correctly (#48).
- `c()` now returns a hms (#41, @qgeissmann).

Documentation and error messages
--------------------------------

- Fix and enhance examples in `?hms`.
- Documentation is in Markdown format now.
- Improved error message if calling `hms()` with a character argument (#29).
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants