gshift works in subset queries #5963
Conversation
MichaelChirico
requested review from
ben-schwen and
mattdowle
as code owners
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5963 +/- ##
=======================================
Coverage 97.49% 97.49%
=======================================
Files 80 80
Lines 14861 14861
=======================================
Hits 14488 14488
Misses 373 373 ☔ View full report in Codecov by Sentry. |
|
@ben-schwen / @jangorecki pinging for review here as we're running up against our CRAN deadline |
|
Looks like this is the last piece before we can ship 1.15.2. If this can be approved tomorrow, I can submit by EOD. |
@TysonStanley please submit regardless -- if this isn't merged in time, we'll simply ship it with a new patch 1.15.4 afterwards. 1.15.2 already includes the urgent fix that CRAN wants, this PR is good-to-have but not strictly required. |
|
Will do. Planning to submit Monday EOD (eastern time). |
|
Merging to |
|
I added the small news from #5950 |
the NEWS doesn't (won't) belong to the 1.16.0 release, though, so I intentionally left it out. This is a slightly strange aspect of the move to patch releases -- cc @jangorecki @tdhock is there anything different we should do here? To summarize the above, we merge this (and generally, any patch-targeted) change to Related but similar issue -- is it possible for a patch release to be superseded by a "normal" release (e.g. we have a branch |
|
As long as we do not mix up numbers between patch release and normal release I do not see anything against a normal release superseding a patch release. I thought that the intended workflow was that we merge everything against master and then cherry pick some merged (smashed) PRs for patch release. If we merge against master without NEWS then we have to keep a trail of non-completed NEWS which seems tedious and error prone |
|
We could merge patch branch to master when it is on CRAN, then at least we will not have changes added without news entities. |
that means excluding the fixes from |
I agree, but the NEWS entries belong under the patch release heading (1.15.2), so it seems the alternative to me is to have NEWS entries for a release that does not exist (at least on CRAN). One more option is migrating the NEWS items from "under development" to "patch" in a post-release commit. That may be the best option but also doesn't feel ideal. |
Not even excluding but simply not merging to master but to a patch branch, and once patch is on CRAN then merging it to master with its news. |
Still not sure I follow. To clarify that means: Day 0, we get a report about a regression bug in a published version That means from days |
|
Yes, it is exactly what I meant. We could be merging patch branch to master even before release to CRAN if it is necessary, but then we need to merge it again after CRAN release. In such case master will temporarily contain patch NEWS items (let's say 1.15.2) before they are on CRAN, and on top of them will be devel NEWS entire (1.15.99). |
|
I'm not a fan of that approach -- it leaves It's not ideal for us, but I think this approach is the friendliest to users:
IINM, this is also more similar to how things have been done historically. |
If that's our way to go then we should probably incorporate it into the CRAN_release checklist |
Definitely! Just want to get some consensus on the approach before applying it here and documenting it. |
|
OK I'm taking the two 👍 on #5963 (comment) as OK to proceed for now. We can easily revisit. |
* add fix for subset * add NEWS --------- Co-authored-by: Benjamin Schwendinger <[email protected]>
Closes #5962. Split off from #5950.