Skip to content

mirror fix to base R for as.IDate#4692

Merged
mattdowle merged 11 commits intomasterfrom
idate_nzchar
May 27, 2021
Merged

mirror fix to base R for as.IDate#4692
mattdowle merged 11 commits intomasterfrom
idate_nzchar

Conversation

@MichaelChirico
Copy link
Copy Markdown
Member

Closes #4676

Actually I wonder if we should branch on the base R capability... for a user on bleeding-edge R-devel, won't they end up running this line of code twice?

is.na(x) = !nzchar(x)

Once in as.IDate.default and then once in as.Date.character?

In that case, we can branch on:

is.na(tryCatch(as.Date(''), error=function(e) 1L))

Or we can store the result of that test in the .global environment.

@codecov
Copy link
Copy Markdown

codecov bot commented Sep 3, 2020

Codecov Report

Merging #4692 (db9324a) into master (4789e55) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head db9324a differs from pull request most recent head 96f5bb6. Consider uploading reports for the commit 96f5bb6 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4692   +/-   ##
=======================================
  Coverage   99.47%   99.47%           
=======================================
  Files          75       75           
  Lines       14807    14809    +2     
=======================================
+ Hits        14729    14731    +2     
  Misses         78       78           
Impacted Files Coverage Δ
R/IDateTime.R 100.00% <100.00%> (ø)

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 4789e55...96f5bb6. Read the comment docs.

@MichaelChirico
Copy link
Copy Markdown
Member Author

Looks like there was some change to the coverage metrics for C code cc @jangorecki

@MichaelChirico
Copy link
Copy Markdown
Member Author

@jangorecki it looks like all (or most) of the changes are things like

default:
    stop(_("Internal error")); // # nocov

The default: line was covered before, isnt' now.

We can either add a // # nocov to the default: line, or we can move the stop() to the default line. Any opinion on which is better?

@jangorecki
Copy link
Copy Markdown
Member

I would start by checking if it is a regression or an enchantment in codecov.

@MichaelChirico
Copy link
Copy Markdown
Member Author

@jangorecki AFAICT no difference in Codecov (not sure how to tell). There may have been a change in gcov or maybe we/they changed gcc version? Also not sure how to tell that.

I don't see anything particularly noteworthy recently in gcov.c, I don't know what else might be the cause:

https://github.com/gcc-mirror/gcc/commits/master/gcc/gcov.c

https://gcc.gnu.org/git/?p=gcc.git&a=search&h=HEAD&st=commit&s=gcov

Comment thread inst/tests/tests.Rraw Outdated
Comment thread R/IDateTime.R Outdated
Comment thread NEWS.md Outdated
Comment thread inst/tests/tests.Rraw Outdated
@mattdowle mattdowle added this to the 1.14.1 milestone May 23, 2021
@mattdowle mattdowle added the WIP label May 23, 2021
@mattdowle
Copy link
Copy Markdown
Member

@MichaelChirico seems like this one is ready, great thanks, but just checking as it's still marked WIP

@MichaelChirico
Copy link
Copy Markdown
Member Author

indeed! sorry about that. removed WIP

@mattdowle mattdowle merged commit b97568a into master May 27, 2021
@mattdowle mattdowle deleted the idate_nzchar branch May 27, 2021 00:57
Comment thread R/IDateTime.R
as.IDate.default = function(x, ..., tz = attr(x, "tzone", exact=TRUE)) {
if (is.null(tz)) tz = "UTC"
if (is.character(x)) {
# 4676 mimics for back-compatibility a similar patch applied to as.Date.character in r79119
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

# 4676 this will not be caught by a regex looking for issue number

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Good spot. Done in df8009d. Since it was just a comment tweak I did that directly without a PR

mattdowle added a commit that referenced this pull request May 28, 2021
@jangorecki jangorecki modified the milestones: 1.14.9, 1.15.0 Oct 29, 2023
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.

as.IDate error that can be fixed

3 participants