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

do not export as.Date.IDate anymore #4782

Merged
merged 7 commits into from
Nov 3, 2020
Merged

do not export as.Date.IDate anymore #4782

merged 7 commits into from
Nov 3, 2020

Conversation

jangorecki
Copy link
Member

closes #4777

@MichaelChirico
Copy link
Member

FWIW i've got a mirror of CRAN locally, statically checked if any revdeps (recursive depends/imports) were using the previous export, didn't find any.

@eddelbuettel
Copy link
Contributor

FWIW comes up too when letting GitHub search in the CRAN mirror: https://github.com/search?q=org%3Acran+as.Date.IDate

@mattdowle mattdowle added this to the 1.13.3 milestone Nov 3, 2020
NAMESPACE Outdated
S3method(as.Date, IDate) # note that zoo::as.Date masks base::as.Date. Both generic.
export(as.Date.IDate) # workaround for zoo bug, see #1500. Removing this export causes CI pipeline to fail on others.Rraw test 6, but I can't reproduce locally.
if (getRversion() >= "3.6.0") {
S3method(as.Date, IDate)
Copy link
Member

@mattdowle mattdowle Nov 3, 2020

Choose a reason for hiding this comment

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

@jangorecki Why >= "3.6.0"? Won't as.Date(x) break in R < 3.6.0 if the method is not exported? At least a comment on why >= 3.6.0 should be added.
Also could the comments be put back so we know there's some history on these lines pointing to #1500 and reminding us that zoo::as.Date masks base::as.Date.

Copy link
Member

@mattdowle mattdowle Nov 3, 2020

Choose a reason for hiding this comment

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

Higher up in the file the switch on 3.6.0 is about delayed registration (i.e. the pkg:: prefix on the first argument to S3method) which was new in R 3.6.0. But that's not the case here with as.Date.

Copy link
Member

@mattdowle mattdowle Nov 3, 2020

Choose a reason for hiding this comment

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

I figured the >=3.6 was copied from higher up. So I just went ahead and removed it in 6a20166, to merge and let's see if it passes GLCI. If this was wrong then we can always do a follow up PR.

Copy link
Member

Choose a reason for hiding this comment

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

When I added the suggest version for zoo, I did bit64 and bit at the same time (although nothing to do with this PR).

@codecov
Copy link

codecov bot commented Nov 3, 2020

Codecov Report

Merging #4782 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4782   +/-   ##
=======================================
  Coverage   99.47%   99.47%           
=======================================
  Files          73       73           
  Lines       14557    14557           
=======================================
  Hits        14481    14481           
  Misses         76       76           

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 1ab3d58...6a20166. Read the comment docs.

@mattdowle mattdowle merged commit 68f6aca into master Nov 3, 2020
@mattdowle mattdowle deleted the s3-lkp branch November 3, 2020 05:20
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.

S3 method lookup found 'as.Date.IDate' on search path
4 participants