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 c, seq, mean methods so ITime retains class #3630

Merged
merged 2 commits into from Jun 12, 2019
Merged

Add c, seq, mean methods so ITime retains class #3630

merged 2 commits into from Jun 12, 2019

Conversation

MichaelChirico
Copy link
Member

@MichaelChirico MichaelChirico commented Jun 5, 2019

Closes #3628

Also deleted split.IDate and cut.IDate methods. They are untested with no examples, and I really don't know what the expected behavior is. split returns a list and really the important part of the method is how to handle the splitting factor f, not the first argument. split.default already retains the IDate class:

dput(split.default(as.IDate(0L), 'foo'))
# list(foo = structure(0L, class = c("IDate", "Date")))

And cut returns a factor so I don't know how as.IDate is supposed to operate on that.

@codecov
Copy link

@codecov codecov bot commented Jun 5, 2019

Codecov Report

Merging #3630 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3630      +/-   ##
==========================================
+ Coverage   98.19%   98.19%   +<.01%     
==========================================
  Files          66       66              
  Lines       12923    12924       +1     
==========================================
+ Hits        12690    12691       +1     
  Misses        233      233
Impacted Files Coverage Δ
R/IDateTime.R 98.75% <100%> (ø) ⬆️

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 421f672...e1767f6. Read the comment docs.

@codecov
Copy link

@codecov codecov bot commented Jun 5, 2019

Codecov Report

Merging #3630 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #3630      +/-   ##
=========================================
+ Coverage   98.19%   98.2%   +<.01%     
=========================================
  Files          66      66              
  Lines       12923   12980      +57     
=========================================
+ Hits        12690   12747      +57     
  Misses        233     233
Impacted Files Coverage Δ
R/IDateTime.R 98.75% <100%> (ø) ⬆️
src/nafill.c 100% <0%> (ø) ⬆️
src/init.c 100% <0%> (ø) ⬆️
R/shift.R 96.42% <0%> (+0.13%) ⬆️

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 421f672...e46f7c6. Read the comment docs.

@jangorecki
Copy link
Member

@jangorecki jangorecki commented Jun 6, 2019

Ideally to raise warning about functions to be deprecated, then in next version remove. Also mention in NEWS.

@MichaelChirico
Copy link
Member Author

@MichaelChirico MichaelChirico commented Jun 6, 2019

In this case, I don't think those functions work at all, so I'm not sure how anybody could have used them... a Note in NEWS maybe

@jangorecki
Copy link
Member

@jangorecki jangorecki commented Jun 6, 2019

I understand but it is unwise just to drop exported functions without any warning.

@mattdowle mattdowle changed the title Closes #3628 -- add c, seq, mean methods so ITime retains class Add c, seq, mean methods so ITime retains class Jun 11, 2019
@mattdowle mattdowle added this to the 1.12.4 milestone Jun 11, 2019
@mattdowle mattdowle merged commit efeee7c into master Jun 12, 2019
4 checks passed
@mattdowle mattdowle deleted the itime_methods branch Jun 12, 2019
@MichaelChirico MichaelChirico mentioned this pull request Sep 11, 2019
31 tasks
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