Closed mattroumaya closed 3 years ago
Hi @sfirke I wanted to check in about this PR and see if it's a possible fix or if I should close. Thanks!
I left comments in a code review. Thanks for creating this. Feel free to add yourself as a contributor on the package's DESCRIPTION.
BTW I left TNTP and no longer use SurveyMonkey (still using R a little though 😄). Dustin could speak to whether TNTP will continue using SurveyMonkey and thus implicitly supporting this package.
I'm glad you're contributing to the project, just clarifying my response times and ability to collaborate.
Thank you @sfirke for the thoughtful code review, and excellent suggestions! I appreciate all of the work that you and Dustin have put into this package, and I have spread the word to my fellow analysts about how great it is. Congratulations on your new job adventure, and I'm happy to continue to contribute whenever there are open issues and/or improvements to be made. Apologies in advance if I've botched anything in this updated push.
Looking good! Thank for being receptive to the review. At this point it's just a matter of the order of a code block so should be 5 minutes to finalize, then I can merge with whatever you go with.
BTW the continuous integration is failing, it looks like an unrelated problem with the Linux error but then the Mac error appears real:
Error in gsub("\r\n", "\n", str, fixed = TRUE) :
input string 1 is invalid in this locale
Calls: <Anonymous> -> new_rcmdcheck -> win2unix -> gsub
Execution halted
But if it's just failing for locale-related reasons on the Mac docker, let's ignore it, as long as it passes check()
locally for you.
Thanks again @sfirke! I made a few additional minor changes while integrating your review recommendations. Also confirming that local check passed (and that I'm on a Windows machine, so unfortunately don't have any insight about the Mac error).
trim_space
is set to TRUE
by default. Couldn't think of a compelling reason why someone would want additional space where there was once HTML tags.trimws
around the gsub
args that remove the additional space, to remove extra space from the ends (gsub
call wasn't working for this instance).Awesome! This is so well-developed now. Thanks for making those last final touches. And hooray for developing R packages on Windows, I feel like the majority of devs (by volume of work at least) are Mac + Linux and having more representation from Windows users would lead to better interoperability sometimes.
Thanks again @sfirke! This is a really amazing package for heavy SurveyMonkey users and I truly appreciate your time spent developing and maintaining it!
Proposed fix for Issue #13 :
ignore
arg that will ignore user-specified values when cleaning column names.