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

Add support for day of week in parse_datetime #763

Merged
merged 1 commit into from Jun 3, 2018
Merged

Add support for day of week in parse_datetime #763

merged 1 commit into from Jun 3, 2018

Conversation

@tigertoes
Copy link
Contributor

@tigertoes tigertoes commented Dec 13, 2017

To keep with other similar libraries, I've chosen %a as the designator. This might cause some issues when doing non-abbreviated day of week in future as %A is already reserved for other uses.

This PR updates the documentation, however none of the generated docs have been updated - is this something expected of contributors? If so, could you please tell me how to generate the documentation.

@jimhester
Copy link
Member

@jimhester jimhester commented Dec 13, 2017

Thanks, this package uses roxygen2 for documentation, so rather than editing the .Rd file you need to edit the relevant code comment, then run devtools::document() to update the Rd file.

If you don't have everything setup to run devtools::document() locally just change the comment and I can run it after I merge.

@tigertoes
Copy link
Contributor Author

@tigertoes tigertoes commented Dec 13, 2017

Thanks Jim, I've generated the docs as you've instructed, however quite a lot of other files have changes as well. Could you look at my most recent commit (b74509be7) and make sure nothing has been put in that shouldn't please?

@jimhester
Copy link
Member

@jimhester jimhester commented Dec 14, 2017

Thanks! I think the added changes are due to using the devel version of roxygen2 rather than the CRAN version. If you could install.packages("roxygen2") it should restrict the changes to only the single file.

If you could also add a test for when using %a with a date that fails to parse that would be helpful!

And please add a note to NEWS.md describing this change and mentioning your GitHub username and this PR #.

Thanks again for working on this!

@tigertoes
Copy link
Contributor Author

@tigertoes tigertoes commented Dec 14, 2017

I'll do all those things, but may I suggest having a CONTRIBUTING file listing all those things out so future contributors know what is expected of them?

@jimhester
Copy link
Member

@jimhester jimhester commented Dec 14, 2017

Yes, I agree and apologize for not having one already. We are working on a standardized one we can use in all tidyverse packages.

@tigertoes
Copy link
Contributor Author

@tigertoes tigertoes commented Dec 18, 2017

Hi Jim, I've made the various changes you've asked for - are you able to take a look at this again please?

@tigertoes
Copy link
Contributor Author

@tigertoes tigertoes commented May 9, 2018

@jimhester Hi mate, it's been nearly half a year since this PR had been looked at. I believe I've addressed all the issues raised, and it's now committed against HEAD as of time of this comment. Could you please take a look again and merge if you're happy?

@tigertoes
Copy link
Contributor Author

@tigertoes tigertoes commented May 29, 2018

Closing this as I've heard no response for over half a year now.

@tigertoes tigertoes closed this May 29, 2018
@jimhester jimhester reopened this Jun 3, 2018
@jimhester
Copy link
Member

@jimhester jimhester commented Jun 3, 2018

Thanks for your patience and for submitting the pull request!

@jimhester jimhester merged commit df61097 into tidyverse:master Jun 3, 2018
2 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants