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

Prefer glue_data() when the data source is a list #1

Merged
merged 1 commit into from
Dec 22, 2023

Conversation

jennybc
Copy link
Contributor

@jennybc jennybc commented Dec 21, 2023

This PR is inspired by doing revdep checks for glue. I'm going to temporarily back off on the associated change in glue, just so I can release without any breakage of other packages.

But please do consider this a heads up that, in the future, glue::glue() will error when .envir is not an actual environment. .envir has always been documented to be an environment and I'd like to make that actually true.

OTOH glue_data() does officially accept something "list-ish" as .x. So I think it's a better choice for your usage.

Backstory in glue:

tidyverse/glue#308
tidyverse/glue@e2b74ff

@jennybc
Copy link
Contributor Author

jennybc commented Dec 21, 2023

Before I rolled back the change in glue, here's what the table.glue check failure looked like for me:

https://github.com/tidyverse/glue/blob/c98f106c420d658da8010627d4a2a4d19f97d758/revdep/problems.md#tableglue

Copy link
Owner

@bcjaeger bcjaeger left a comment

Choose a reason for hiding this comment

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

Wow - what an incredibly thoughtful PR. The changes make sense, especially given the background you provide, and I'm happy to merge and then push the update to CRAN. Thank you!

@bcjaeger bcjaeger merged commit 18966e1 into bcjaeger:master Dec 22, 2023
@jennybc
Copy link
Contributor Author

jennybc commented Jan 10, 2024

I released glue 1.7.0 today without this change and then re-introduced the change. So you can expect it to be present in glue's next release, which I have no concrete plans for. But barring an unforeseen release in the next ~2 weeks, I will consider this issue as me having given plenty of notice of the change.

This also means you can check your package in the presence of dev glue to test your own fix for the issue.

@jennybc jennybc mentioned this pull request Sep 19, 2024
16 tasks
@jennybc jennybc deleted the prefer-glue-data branch September 19, 2024 20:29
@jennybc
Copy link
Contributor Author

jennybc commented Sep 19, 2024

At long last I am about to make a glue release. I think this PR was merged but this is not available in the CRAN version of table.glue yet? I still see it in my failed revdeps:

https://github.com/tidyverse/glue/blob/main/revdep/problems.md#table.glue

So this is a just another heads up. Some new revdeps have cropped up since I last made all the PRs, so it will take me a little while to submit. So you would be able update on CRAN before this new glue version hits and breaks table.glue.

@bcjaeger
Copy link
Owner

Sorry about that. Yes, version 0.0.4 is ready, but I forgot to submit it after merging this PR. I just submitted it to CRAN. Thank you for following up and for all you've done with glue!

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