search

eddelbuettel / nanotime

Nanosecond Resolution Time Functionality for R
https://eddelbuettel.github.io/nanotime
GNU General Public License v2.0
53 stars 7 forks source link

Comparaison nanoduration and character #88

Closed statquant closed 3 years ago

statquant commented 3 years ago

Following #87 here is a PR that allows comparaison between nanoduration and character. Functionality-wise I think it makes sense as it mimics nanotime behavior and base R Date behavior. It builds locally and passes all tinytests. I had to modify 2 tests that would have failed. If you're ok with adding the functionality please let me know if I should check/change/fix anything. Regards

eddelbuettel commented 3 years ago

Thanks! From a first glance that looks spot on. (I even saw you fork and looked briefly and saw the first commit so thanks for the second -- my use of roxygen2 would just have overwritten ...)

As you know I tend to document each change in each file (apart from very trivial ones) via ChangeLog. If you would like me to use an email that different than the one that comes up (with your work address) let me know. An anonymous PR will not go as that is simply not how we roll. Users of the code deserve to know where it comes from.

eddelbuettel commented 3 years ago

(Oh, btw, la comparaison becomes the the comparison. I fight with English too as a non-native speaker...)

statquant commented 3 years ago

Hello, With regard to required tests: I have added 2 sets of tests testing for ==, !=, >, >=, <, <= and they passed locally With regard to ChangeLog: Thanks for bringing the email to my attention, I rebased and changed the email to the one being displayed on my profile. My understanding is that you require my name in git log, so I left it. Thanks

Edit: I see checks failing, I am now afraid that my git commit --amend --author ... broke something. Happy to start a new PR from scratch if it proves useful.

eddelbuettel commented 3 years ago

Looks good, thanks! I had the pr checked out here (emacs magit makes that so easy...), and your rebase/force broke that but I just started over.

Thanks for adding those tests, that is very nice too. I'll look into adding a quick changelog entry. And will look into the failed Travis builds, looks like a spurious setup issue (which happens once in a while). It passes locally for me here.

eddelbuettel commented 3 years ago

I think we're there---I keep pushing into your branch now for simplicity. My Travis CI credits may be up, so I am now adding a runner at GitHub Actions.

eddelbuettel commented 3 years ago

Ok, and rolled the minor version to distinguish the tar ball.

Your corresponding ChangeLog change is in there too.