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

Switch to tibble::set_tidy_names() from tibble::repair_names() #453

Merged
merged 1 commit into from
Apr 25, 2018

Conversation

jennybc
Copy link
Member

@jennybc jennybc commented Apr 15, 2018

Resolves #357

Recall previous discussion about name repair, that lead to tibble::set_tidy_names(): tidyverse/tibble#217

I would really like to make this change. And I predict that revdep checks will reveal few/no problems with this move in packages.

But I also know from the previous release that any change here generates a lot of upset and issues, as people have scripts that are hardwired to whatever the previous name repair strategy was. It also always brings up the usual suggestions to clean up nonsyntactic names. Quoting @hadley from thread linked above:

I'm not sure what we should do with respect to backward compatibility. I suspect few packages depend on this behaviour; it's going to be more user code. I think as long as there's a clear way to get to the old behaviour it shouldn't cause much hassle (especially since we're now describing what's happening)

Thoughts on whether and how to proceed?

NEWS.md Outdated
@@ -1,5 +1,7 @@
# readxl 1.0.0.9000

* Missing or duplicated column names are now repaired with `tibble::set_tidy_names()` in `read_excel()` and friends. `set_tidy_names()` is intended to implement a standard across the tidyverse packages for repairing missing and duplicated names. Its design is discussed in [tidyverse/tibble#217](https://github.com/tidyverse/tibble/issues/217). (PR here, #357)
Copy link
Member

Choose a reason for hiding this comment

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

You might also want to include as a heading under a "Breaking changes" section where you describe the issue from the symptoms (i.e. how have names changed), and offer a work around in code.

Copy link
Member Author

Choose a reason for hiding this comment

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

Roger re: "Breaking changes"

offer a work around in code

Well that's the problem. I don't really offer one right now. To offer a nice one ... I think I'd need to add an argument that affects name repair. Is it worth it?

The only workaround possible with the current strategy would be a post hoc regex-based renaming strategy, given what I know about the old and new repaired names.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't you need to have the workaround in the package functions, instead you can provide code for one in the NEWS.md + release blog post.

@jennybc
Copy link
Member Author

jennybc commented Apr 16, 2018

I'm going to hold off on merging this, but will foreshadow in NEWS/blog post.

Will do shortly after release, so people have time to adjust. Would be obvious to combine in a CRAN release with col spec improvements.

@krlmlr
Copy link
Member

krlmlr commented Apr 17, 2018

pkgconfig works for me in these situations: We could start with opt-in even before CRAN release, and just switch the default after CRAN release.

CC @gaborcsardi.

@jennybc jennybc merged commit a7bfa4c into master Apr 25, 2018
@jennybc jennybc deleted the set-tidy-names branch April 25, 2018 01:03
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.

4 participants