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

Added assignment operators to newDateVector and newDatetimeVector #587

Merged
merged 4 commits into from Nov 14, 2016

Conversation

@dcdillon
Copy link
Contributor

@dcdillon dcdillon commented Nov 13, 2016

No description provided.

@dcdillon
Copy link
Contributor Author

@dcdillon dcdillon commented Nov 13, 2016

Might be nice to have a plugin to turn this functionality on as well.

@eddelbuettel
Copy link
Member

@eddelbuettel eddelbuettel commented Nov 13, 2016

this functionality

What are you talking about, exactly? Is it this:

edd@max:~/git/rcpp(master)$ git diff
diff --git a/inst/include/Rcpp.h b/inst/include/Rcpp.h
index d365958..fbae1c1 100644
--- a/inst/include/Rcpp.h
+++ b/inst/include/Rcpp.h
@@ -57,6 +57,7 @@
 #include <Rcpp/DataFrame.h>

 //  #define RCPP_NEW_DATE_DATETIME_VECTORS 1
+#define RCPP_NEW_DATE_DATETIME_VECTORS 1
 #include <Rcpp/date_datetime/date_datetime.h>

 #include <Rcpp/Na_Proxy.h>
edd@max:~/git/rcpp(master)$ 

That is a single -D... argument to the compiler in ~/.R/Makevars, as an env.var or, like here, as edit. Do we really need a plugin for that?

@dcdillon
Copy link
Contributor Author

@dcdillon dcdillon commented Nov 13, 2016

I just think it makes it cleaner than relying on the developer for it. Then we can also change how it's implemented behind the scenes (if needed) and change the plugin accordingly to get the right build configuration.

@eddelbuettel
Copy link
Member

@eddelbuettel eddelbuettel commented Nov 13, 2016

Very nice that you added unit tests too.

@eddelbuettel
Copy link
Member

@eddelbuettel eddelbuettel commented Nov 13, 2016

Calling for reviews or thumbs up/down ... might be nice to get this in before I run a round of tests tomorrow.

@kevinushey @thirdwing @nathan-russell @jjallaire

@kevinushey
Copy link
Contributor

@kevinushey kevinushey commented Nov 13, 2016

LGTM, although I'm not as familiar with datetime handling (+ the associated traps inherent) with R.

@eddelbuettel
Copy link
Member

@eddelbuettel eddelbuettel commented Nov 13, 2016

There is not much too it, but it looks super-confusing. POSIXct is just out friend time_t, generalised by BDR et al to be a 53 digit double (hence with about microsec precision from the remaining bits). It generally stores in UTC as one should and uses a TZ attribute for offset (when formatting for humans, say).

I didn't understand what attributes did eight or nine years ago so the first iteration of these were std::vector<> containers which is very very un-SEXP. So now we use NumericVector with a class attribute, and the world is a better place. There aren't too many packages using these types, and some I am involved with so the transition should be easy. (And POSXlt, as you know, is just lists of struct tm components presenting the same content.)

And what Dan did here was filling gaps I had left as I had, ahem, forgotten that constructors etc don't inherit. So we needed a bit of additional stuff which we now have.

@eddelbuettel eddelbuettel merged commit 019fefd into RcppCore:master Nov 14, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.