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

feat: '+' / '-' methods for {nanotime} objects and 'difftime()' objects #110

Merged
merged 5 commits into from
Oct 18, 2022
Merged

Conversation

trevorld
Copy link
Contributor

Adds the following S4 methods:

* nanoduration() + difftime()
* difftime() + nanoduration()
* nanoduration() - difftime()
* difftime - nanoduration()
* nanotime() + difftime()
* difftime() + nanotime()
* nanotime() - difftime()
* nanoduration() + nanotime() (arguably missing S4 method)

closes #107, closes #108

* nanoduration() + difftime()
* difftime() + nanoduration()
* nanoduration() - difftime()
* difftime - nanoduration()

closes #107
* nanotime() + difftime()
* difftime() + nanotime()
* nanotime() - difftime()
* nanoduration() + nanotime() (arguably missing S4 method)

closes #108
Copy link
Owner

@eddelbuettel eddelbuettel left a comment

Choose a reason for hiding this comment

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

Thank you -- that too looks good. I will wait for @lsilvest to nod as well before merging.

@lsilvest
Copy link
Collaborator

If we do have difftime for nanotime and nanoduration, I think for coherence we also want to do it for nanoival, so for that type too we can add/subtract directly a difftime.

Also I would ideally prefer the unit tests to be distinct for each function added. Admittedly they are not very complicated, but one never knows what can break down the line and hence if one will need these separate unit tests.

* nanoival() + difftime()
* difftime() + nanoival()
* nanoival() - difftime()
* nanoduration() + nanoival() (arguably missing S4 method)
* nanoival() - nanoduration() (arguably missing S4 method)
@trevorld
Copy link
Contributor Author

@lsilvest Each new method now has its own individual test and I've added the following new S4 nanoival() methods:

* nanoival() + difftime()
* difftime() + nanoival()
* nanoival() - difftime()
* nanoduration() + nanoival() (arguably missing S4 method)
* nanoival() - nanoduration() (arguably missing S4 method)

* Adds a 'setOldClass("difftime")' which we probably "should" do
* Eliminates some warnings when running 'devtools::load_all()'
  but otherwise no detectable change (i.e. in 'R CMD check')
@eddelbuettel
Copy link
Owner

eddelbuettel commented Oct 18, 2022

Thank you @trevorld for the added (and updated) PR and to @lsilvest for catching that nanoival was needed too.

If this completes the set of changes @trevorld is planning to bring I can look into a quick release in a few days. (Got a few emails from CRAN about clang-15 changes I should chew on first so likely early next week for this if we are good with that.)

@eddelbuettel eddelbuettel merged commit 950f566 into eddelbuettel:master Oct 18, 2022
@trevorld
Copy link
Contributor Author

This completes the set of changes I was planning.

@eddelbuettel
Copy link
Owner

I can look into a quick release in a few days

Got around to that late yesterday, and by now Uwe processed and the new version is on CRAN.

Many thanks again to @trevorld for all the careful patches in it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants