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

Date coercion #389

Closed
wants to merge 11 commits into from
Closed

Conversation

imanuelcostigan
Copy link
Contributor

Address #355:

  • New as_date generic
  • Methods for POSIXt, numeric and character vectors (per as.Date S3 methods) with sensible defaults
  • Generic and methods are documented.

NB:

  • Check passes except for one note regarding the following warning message:
@details [periods.r#1]: mismatched braces or quotes 

@@ -608,3 +608,43 @@ setMethod("as.character", signature(x = "Duration"), function(x, ...){
setMethod("as.character", signature(x = "Interval"), function(x, ...){
format(x)
})


#' Change an object to a Date
Copy link
Member

Choose a reason for hiding this comment

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

It should probably be "convert".

@vspinu
Copy link
Member

vspinu commented Mar 11, 2016

As to the commiting style, could you please keep related changes in one commit whenever possible? No need to split out documentation, roxigenization and news addition steps. All of these are a single change - adding one feature.

Also could you please add a test case as per #355?

@vspinu
Copy link
Member

vspinu commented Mar 28, 2016

Hi Imanuel. What is the status of this? Do you plan to address my other comments and add tests? I would like to release by the end of this week. I can merge what you have so far and amend those myself if you don't have time.

@imanuelcostigan
Copy link
Contributor Author

@vspinu I'm unlikely to get time before end of the week. In terms of commit style - sure. Suggest that you consider adding a CONTRIBUTING file to repo with this and other expectations.

@vspinu
Copy link
Member

vspinu commented Mar 29, 2016

Ok. I will then merge manually your changes these days then leave for another shelve for 5-6 days before the final release. Will add the CONTRIBUTING file. Thanks for the idea.

@imanuelcostigan
Copy link
Contributor Author

@vspinu found myself with some time tonight. Have added test cases and default method as requested.

@vspinu
Copy link
Member

vspinu commented Mar 30, 2016

There are merge conflicts. I am merging it right now. By "default" I meant to use useAsDefault argument to setGeneric, but I guess this doesn't make any difference.

BTW, why do you need a method for "character"? as.Date does good job there. No?

@imanuelcostigan
Copy link
Contributor Author

as.Date fine in that respect, but why not give users the same function call for everything? Of course, there is a performance hit by wrapping as.Date in an S4 method call, but I think users will appreciate unified call semantics. If they want performance, they can always call as.Date directly.

@vspinu
Copy link
Member

vspinu commented Mar 30, 2016

Performance is not an issue. We are defaulting to as.Date anyways. So character input will drop into as.Date.character which seem to be doing a bit more than format = "%Y-%m-%d".

@vspinu vspinu closed this in a78509a Mar 30, 2016
@imanuelcostigan
Copy link
Contributor Author

Btw see https://github.com/blog/2141-squash-your-commits for setting your repo up to squash commits rather than requiring contributors to do so

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.

2 participants