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

Feature/new date and datetime vector classes (closes #34) #557

Merged
merged 8 commits into from Oct 18, 2016

Conversation

@eddelbuettel
Copy link
Member

commented Oct 16, 2016

This PR is, in aggregate, not changing anything because the 'new' part is not (yet) activated.

The switch is in the (new) header file inst/include/Rcpp/date_datetime/date_datetime.h which regroups all #include for Date/Datetime and their vector variants.

For now, we typedef the old and existing code in. I think this can be merged as it allows for more testing. We can allow for the usual one-year deprecation (or accelerate it if we feel the change was so trivial).

I use these in my packages RQuantLib, Rblpapi, and anytime -- and want the new and better DatetimeVector (a reclassed `NumericVector) there. T he rev.dep checks I will do may tell me a bit more about other packages using this.

new DateVector & DatetimeVector
tests passed, need to figure out pre-release how to phase this in
will run at least one full rev.dep check with this on
@eddelbuettel eddelbuettel changed the title Feature/new date vector Feature/new date and datetime vector classes Oct 16, 2016
@eddelbuettel

This comment has been minimized.

Copy link
Member Author

commented Oct 16, 2016

@kevinushey @thirdwing @jjallaire : If you could have a look at this one which in essence
-- rename DateVector and DatetimeVector by prefixing old* to them
-- keeps them as the default
-- regroups several related header files in one new directory
-- adds a new variant which was tested in the 3rd of 4 commits and then turned off to have the existing behaviour be the default
All this should be pretty vanilla. We could merge and then rebased for KK's WIP PR.

default is empty string == not set which implies local timezone
that is what we had before, and what R does
@eddelbuettel

This comment has been minimized.

Copy link
Member Author

commented Oct 17, 2016

Running a full rev.dep now with the 'new' classes turned on. All good so far, not a single new failure. Currently at 130 out of 801 with 128 good and two (know, repeat) offenders. Will report back this evening.

Paging @kevinushey @thirdwing @jjallaire for a look at this.

Also: full-blown one-year deprecation, or is this more of a 'same API internal representation change' that we could release as is? Thoughts?

@eddelbuettel

This comment has been minimized.

Copy link
Member Author

commented Oct 18, 2016

Passed with flying colours; 794 ok, 7 fails of which 4 happened before, 1 happens with release version of Rcpp, 1 lacked an external (web resource) and 1 was spurious (package upgraded while test ran).

@eddelbuettel

This comment has been minimized.

Copy link
Member Author

commented Oct 18, 2016

So I plan to merge to merge this in short little while.

@kevinushey : comments now or after the fact :)

// this will not be on by default
#if defined(RCPP_NEW_DATE_DATETIME_VECTORS)

typedef oldDateVector DateVector;

This comment has been minimized.

Copy link
@kevinushey

kevinushey Oct 18, 2016

Contributor

It looks like the two #ifdef blocks here are identical; is the intention for these to be newDateVector?

This comment has been minimized.

Copy link
@eddelbuettel

eddelbuettel Oct 18, 2016

Author Member

Oops.

private:

void setClass() {
Rf_setAttrib(*this, R_ClassSymbol, Rf_mkString("Date"));

This comment has been minimized.

Copy link
@kevinushey

kevinushey Oct 18, 2016

Contributor

Although it might not be a problem here, Rf_mkString() should be protected before being passed to Rf_setAttrib() (otherwise if Rf_setAttrib() did any R allocation that "Date" SEXP could be collected)

This comment has been minimized.

Copy link
@eddelbuettel

eddelbuettel Oct 18, 2016

Author Member

Sure. I'll assign to Shield<SEXP> first, then call Rf_setAttrib().

Copy link
Member Author

left a comment

Will make those changes, thanks!

// this will not be on by default
#if defined(RCPP_NEW_DATE_DATETIME_VECTORS)

typedef oldDateVector DateVector;

This comment has been minimized.

Copy link
@eddelbuettel

eddelbuettel Oct 18, 2016

Author Member

Oops.

private:

void setClass() {
Rf_setAttrib(*this, R_ClassSymbol, Rf_mkString("Date"));

This comment has been minimized.

Copy link
@eddelbuettel

eddelbuettel Oct 18, 2016

Author Member

Sure. I'll assign to Shield<SEXP> first, then call Rf_setAttrib().

with thanks to @kevinushey
@eddelbuettel

This comment has been minimized.

Copy link
Member Author

commented Oct 18, 2016

Currently more than 50% done on a second rev.dep run with these changes.

roll Version, Date
@eddelbuettel

This comment has been minimized.

Copy link
Member Author

commented Oct 18, 2016

Just committed the summary to rcpp-logs: five new build failures. Three of which get fixed with a one-line addition I already made (providing operator double() for the Date class); the other two are RQuantLib and RcppExamples which are both my packages --- and with the planned deprecation I have a year to fix that. More than ample.

So I will merge the PR later today unless somebody screams real loud and real soon . 😀

@eddelbuettel eddelbuettel changed the title Feature/new date and datetime vector classes Feature/new date and datetime vector classes (closes #34) Oct 18, 2016
@eddelbuettel eddelbuettel merged commit 40dde2f into master Oct 18, 2016
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@eddelbuettel eddelbuettel deleted the feature/new_date_vector branch Oct 18, 2016
@eddelbuettel eddelbuettel referenced this pull request Oct 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.