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 data might be list-ish, not an actual environment #310

Merged
merged 1 commit into from
Jan 10, 2024

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

@richfitz
Copy link
Member

Thanks Jenny - I'll get this sorted shortly in the new year. For various reasons the CRAN version is miles behind these sources and I'll backport these changes to the CRAN branch so that I can get it released there if this ends up being an issue for you.

Copy link

codecov bot commented Dec 22, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c51efee) 99.98% compared to head (4683f20) 99.98%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #310   +/-   ##
=======================================
  Coverage   99.98%   99.98%           
=======================================
  Files          48       48           
  Lines        5560     5559    -1     
=======================================
- Hits         5559     5558    -1     
  Misses          1        1           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jennybc
Copy link
Contributor Author

jennybc commented Jan 4, 2024

Sounds good! I didn't get glue released before the CRAN holiday hiatus anyway. But I am going to release when it reopens, without the change that breaks odin (and a couple of other packages).

Then I'll re-implement the change in the dev version of glue. So presumably we're just laying the groundwork for a smooth release of glue in the future, in terms of revdep breakage.

@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.

@richfitz richfitz merged commit 1344908 into mrc-ide:master Jan 10, 2024
10 checks passed
@jennybc jennybc deleted the prefer-glue-data branch September 19, 2024 02:28
@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 odin yet? I still see it in my failed revdeps:

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

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 odin.

@jennybc jennybc mentioned this pull request Sep 19, 2024
16 tasks
@richfitz
Copy link
Member

Thanks Jenny. I'm in the middle of rewriting this whole thing, but I'll do an update for CRAN tomorrow or next week. I've been dealing with strict headers this week so I have my systems fairly dialed and flameproof clothing handy

@richfitz
Copy link
Member

This is on CRAN now

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