Skip to content

Comments

Support seq() for hms values#162

Merged
krlmlr merged 9 commits intotidyverse:mainfrom
DivadNojnarg:f-seq-support
Mar 31, 2025
Merged

Support seq() for hms values#162
krlmlr merged 9 commits intotidyverse:mainfrom
DivadNojnarg:f-seq-support

Conversation

@DivadNojnarg
Copy link
Contributor

Currently seq() fails on 'hms' objects due to 'hms' inheriting from 'difftime' and 'difftime' being difficult to work with generally. seq() being a S3 generic we can however define a method for 'hms' where we use vctrs::vec_cast(, numeric()) on its from, to, by arguments. And reconvert to 'htm' before returning.

@krlmlr: we had to use bquote to splice the parameters gathered in the ... as we can't use !!! with seq. It seems quite daunting. Maybe you have something easier?

Closes #117

@krlmlr
Copy link
Member

krlmlr commented Mar 26, 2025

Thanks. One way to do this with rlang:

args <- list(from = 1, to = 3, by = 0.5)

rlang::inject(seq(!!!args))
#> [1] 1.0 1.5 2.0 2.5 3.0

Created on 2025-03-26 with reprex v2.1.1

@DivadNojnarg
Copy link
Contributor Author

DivadNojnarg commented Mar 26, 2025

Not sure about the reason of the failure, as it does not seem to be related to the new seq.hms

edit: actually maybe because we needed a dev version of roxygen 2

Found the following Rd file(s) with Rd \link{} targets missing package
  anchors:
    hms.Rd: vec_cast
  Please provide package anchors for all Rd \link{} targets not in the
  package itself and the base packages.

@krlmlr krlmlr changed the title Support seq() for hms values Support seq() for hms values Mar 26, 2025
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, great. If we must use inject(), we can, but do we really have to?

R/hms.R Outdated
Comment on lines 283 to 286
seq.hms <- function(
from = hms(1),
to = hms(1),
...) {
Copy link
Member

Choose a reason for hiding this comment

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

Minor nit: we prefer single indent these days.

Suggested change
seq.hms <- function(
from = hms(1),
to = hms(1),
...) {
seq.hms <- function(
from = hms(1),
to = hms(1),
...
) {

Is there a reason to not add by to the signature to avoid handling the dots?

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 refactored the code to have by in the signature and get rid of ... handling.

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, nice!

DivadNojnarg and others added 2 commits March 27, 2025 10:18
Co-authored-by: Kirill Müller <krlmlr@users.noreply.github.com>
Co-authored-by: Kirill Müller <krlmlr@users.noreply.github.com>
@DivadNojnarg
Copy link
Contributor Author

@krlmlr Let me know if you need anything else on this PR.

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, merging.

Comment on lines +300 to +311
if (!is.null(by)) {
if (!(is_hms(by) || inherits(by, "difftime"))) {
abort(sprintf(
"`by` isn't of class `hms` or `difftime` (current class: `%s`).",
class(by)[1]
))
}
by <- vec_cast(as_hms(by), numeric())
return(hms(seq(from, to, by, ...)))
}

hms(seq(from, to, ...))
Copy link
Member

Choose a reason for hiding this comment

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

Minor nitpick: if we check if (is.null(by)), the shortcut (immediate return) is much easier to follow.

@krlmlr krlmlr merged commit 4a230ec into tidyverse:main Mar 31, 2025
17 checks passed
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.

Support seq() for hms values

2 participants