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

Integrate snakecase-pkg into janitor #131

Merged
merged 2 commits into from Sep 21, 2017
Merged

Integrate snakecase-pkg into janitor #131

merged 2 commits into from Sep 21, 2017

Conversation

Tazinho
Copy link
Contributor

@Tazinho Tazinho commented Aug 16, 2017

As mentioned in #96 and #130 For the latter I changed "%" into ".percent_" instead of "_percent_", as commented in the code, since starting with "_" would have been converted to "x_percent" by make.names() .

@sfirke
Copy link
Owner

sfirke commented Aug 23, 2017

Looks good. I have two questions:

  1. Test coverage: I'd like there to be tests for each of the case options. Are there similar tests from snakecase that we could quickly incorporate? It would be a bit repetitive but we could start by just taking the existing set of clean_names tests and test it under each value of case, that would be quick.

  2. The part in the documentation where it notes 'There are three "special" cases available' - do you think those will be appealing to users in this use case, automatically reformatting their variable names? I lean toward omitting that part for brevity.

Thanks Malte!

@Tazinho
Copy link
Contributor Author

Tazinho commented Aug 23, 2017

Hi Sam,

  1. There are many (more or less systematic) testcases in tests-to_any_case.R. I included your testcases for the case "snake" under the name janitor. I am currently on vacation, but afterwards will update these tests for the other cases. This should be straightforward and there should be no surprises. I wasnt sure if yould like to see the tests on my side, but I can include more tests and I'd be happy to offer support for any issues dealing with cases.

  2. Hard to tell, but from a typical janitor usecase I'd expext that only snakecase, Upper/lower camel and all caps matter. Maybe parsed and mixed case also. I'd like to leave it up to you to edit the description.

Best, Malte

@Tazinho
Copy link
Contributor Author

Tazinho commented Sep 5, 2017

Testcases for all available cases now included within
https://github.com/Tazinho/snakecase/blob/master/tests/testthat/test-to_any_case.R

@sfirke
Copy link
Owner

sfirke commented Sep 8, 2017

Thanks for adding the additional test cases! I'll tinker with the description per your comment above, re-read the code, hopefully this week, and then merge it.

@sfirke
Copy link
Owner

sfirke commented Sep 13, 2017

Okay if I copy those tests to janitor? Seems like they should call clean_names() directly in case it changes in the future.

@Tazinho
Copy link
Contributor Author

Tazinho commented Sep 14, 2017

Definitely ok.

@sfirke sfirke merged commit 79b1874 into sfirke:master Sep 21, 2017
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.

None yet

2 participants