Finalizing changelog for v4.3rc1#11856
Conversation
|
👋 Thank you for your draft pull request! Do you know that you can use |
|
Wow the labeling bot went a bit nuts with this one. Not inaccurate, but kinda amusing! |
|
I have to recuse myself from this one. I still don't grok towncrier. @saimn is the best person to ask. |
bsipocz
left a comment
There was a problem hiding this comment.
The RC certainly should have a changelog shipped with it. But! I would keep the previous approach of updating the heading from 4.3.rc1 to 4.3 for the release, so for the actual release there won't be an RC entry.
(maybe that means that we should not delete the snippets? I don't know, my take is that once the changelog is rendered they are not relevant anymore and should be deleted).
I left a couple of comments, it seems that we have some minor formatting issues that maybe a pre-commit could resolve easily for contributors, but otherwise we just need to keep doing manual cleanups.
The most important that the 4th section, "Other changes" is not right, it should not have any module level content.
Also, I noticed that entries got much much longer with narrative words that brings no meaning, I much more like the terser approach we had before, keep the fluffy descriptive sentences into the what's new and into the description of the PR for those who care enough to look under the hood (after all we list the PR number in the changelog for a reason).
| Fitters that support constraints will set this to False on the model copy | ||
| and then set back to True when the fit is complete before returning. [#11365] | ||
|
|
||
| - The ``convolve_models_fft`` function implements model convolution so that one insures that the convolution remains consistent across multiple different inputs. [#11456] |
There was a problem hiding this comment.
reformat, so no lines go over the 80 chars, some of the ones above may also be longer than 80.
TODO: we should do a pre-commit that cuts the towncrier snippets into multi lines, as people will be always run over otherwise
There was a problem hiding this comment.
(I fixed these cases by hand for now but I agree it's a great idea to automate this, so not resolving this one)
| component names, e.g. having both columns “dec” and “pm_dec” no longer errors | ||
| and will be included in the SkyCoord. [#11417] | ||
|
|
||
| - All representations now have a ``transform`` method, which allows them to be |
There was a problem hiding this comment.
Some of these changelog entries are unusually long, maybe it's a side effect of towncrier? Overall I would reemphasize that these should be much briefer, keep the details for the what's new.
There was a problem hiding this comment.
Yeah, a good point. I'm guessing it's the psychology that you don't see a bunch of other bullets that are short and to the point? So yeah, emphasizing briefer is a good idea. Maybe we should update the PR guidelines for this or something?
|
|
||
| - Fix a bug that caused APE 14 WCS transformations for FITS WCS with ZOPT, BETA, VELO, VOPT, or VRAD CTYPE to not work correctly. [#11781] | ||
|
|
||
| Other Changes and Additions |
There was a problem hiding this comment.
Other changes and additions should not have module specific entries, this section is really just for the extra stuff
There was a problem hiding this comment.
Yeah, that's weird. I don't really understand how town crier decided that, but I will just manually fix it for now.
There was a problem hiding this comment.
towncrier doesn't decide ;), it's up to people to put the fragment in the correct location or to a reviewer/bot to check that 😬
So fragments for "Other changes and additions" should go in docs/changes/, and if they are put in a sub-directory they will be rendered in a sub-category.
❯ ls -1 docs/changes/*/*other*
docs/changes/coordinates/11069.other.rst
docs/changes/coordinates/11073.other.rst
docs/changes/coordinates/11090.other.rst
docs/changes/coordinates/11449.other.rst
docs/changes/coordinates/11485.other.rst
docs/changes/coordinates/11601.other.rst
docs/changes/coordinates/11753.other.rst
docs/changes/cosmology/11530.other.rst
docs/changes/io.ascii/11919.other.rst
docs/changes/nddata/11107.other.rst
docs/changes/nddata/11553.other.rst
docs/changes/stats/11219.other.rst
docs/changes/stats/11356.other.rst
docs/changes/utils/11490.other.rst
There was a problem hiding this comment.
It seems other was chosen a lot for changes that looked neither like a bugfix nor like a feature, but not sure where those changes would have belong before ?
There was a problem hiding this comment.
there is no such thing as not a bugfix nor a feature nor an API change. Then those are refactoring and thus internal and has no place in the changelog 🤷 I suspect this really is just a side effect of not reviewing those entries/name of files such as it was the practice before (e.g. I don't think mainainers would have let an entry go through in "Other" category before, and also people didn't think it's a category as it wasn't prepopulated with module names). So, we just need to clean up now at release.
Also, @eteq, keep in mind that with towncrier there are no examples provided for contributors for how a changelog entry looks like, so there is a much large spread of quantity and quality, thus more manual cleanup is needed at release time. Which I think is OK, we minimized on contributor convenience and PR conflicts not release easiness. So, the release manager certainly need to step in a clean up much more than before (note: there was quite a bit of cleanup to he changelog before, too.)
There was a problem hiding this comment.
we may want to trace a few of those "other" cases and find out how those contributors first found there way there, since that will let us see where we could have intervened to put them on the happier path
There was a problem hiding this comment.
the tradeoff is days worth of work in terms of backport, and nitpicking on wordsmithing for the changelog. I don't think the latter should be a bottleneck, but of course would be nice if maintainers were picky at the time of PR opening to catch issues. But I don't think it's a have a cake and eat is situation, without rendering and seeing (not just listing in the dev docs) of good, compact examples people will just write long fluffy sentences that are not following those guidelines.
Again, with towncrier we traded the conflicting PRs due to the changelog, the task to figure out the right version number section etc for someone to have a final look and cleanup at the end. It's a good tradeoff, could be better if maintainers are more nitpicky, but not a catastrophic situation to be in either.
There was a problem hiding this comment.
Agreed, the drawbacks we see here are a bit annoying but that remains a minor annoyance compared to the benefits. Now I guess the question is how to improve this for future releases, it would be great if we could fix the bot, also maybe we can add some checks with pre-commit.
There was a problem hiding this comment.
Right, to be clear I was not arguing against towncrier, just that increasing the workload on the release managers was one of the things we were trying to not do. But I agree there's a have cake and eat it too solution, just have to decide which!
There was a problem hiding this comment.
Yep. Just to mention it here, #11945 adds a pre-commit config that would check those towncrier filenames and avoid this issue in the future.
|
Since we don't keep a separate changelog for rc releases in the final release, I guess what I would do is:
But there may be other ways to do it. |
I'm torned which approach is best, regenerating it would mean that any fixes that are made are lost (see my list above, there should be quite a few done).
I always made the fixes in the releases and then "forward" port the changelog fixes (not a proper forward port, but a PR that's not backported). Now it's a bit fuzzy what happened for 4.3.rc1, and what the purpose of this PR. The release is out without the fixed from this PR, yet this one is not merged either. |
|
About fixing typos, yeah this is a bit annoying, not sure which is the best way to proceed. I don't understand your solution (prior tagging RC etc.) ? |
Then it should not be milestoned to 4.3. Also, there are a ton of issues with it, e.g it's not exactly the same version that went in the RC as a few fixes have been done there, and we suggest a few fixes here, and we likely will end up with divergent versions on the two branches. |
Yeah, I mostly like this. But to limit the number of back-and-forth PRs, I suggest a PR like this one as the place where review can be done between when the RC is out and the final release (that's was sort of my "second" option in the OP). So to be concrete, this is what I'm thinking should be the process:
So as not to delay 4.3 further I'm going to follow this process for this PR (i.e., accept @bsipocz's comments - thanks for the review! - and then backport), with a manual fixup step to work around the fact that I didn't follow 1-3 exactly (fortunately nothing backported after the rc seems to have a changelog entry). But if we want to discuss further, we can modify it in the future! We'll need to write down the answer in the docs a la #11847, regardless, though. EDIT: after some more thought I realized this might not be so great because it does not sychronize well with the LTS release... but 🤷 I'm not sure what to do about that right now and 4.3 has already been waiting too long on figuring some of this out. |
|
Haha, the github action is unhappy that I'm editing the changelog even though I marked it |
|
@bsipocz, can you have a quick look at the updates and approve if this looks good to you now? (it's holding up 4.3 release, which is why I milestoned for 4.3). |
|
Ops... |
we used to have a label to ignore those PRs. At least this was worked around before |
We did with |
bsipocz
left a comment
There was a problem hiding this comment.
more nitpicking of long lines (there maybe a few more I haven't notice, and a few inconsistencies, but overall this is now OK).
b18cc3e to
3e20112
Compare
|
had to rebase to integrate one more late-breaking backport needed to fix the CI in 4.3.x (#11921) |
Co-authored-by: Brigitta Sipőcz <b.sipocz@gmail.com>
b64b4bb to
4dfc510
Compare
pllim
left a comment
There was a problem hiding this comment.
LGTM. Thanks!
p.s. One thing I don't like about towncrier stuff is that we still have to make sure the rendered change log is correct at release time. In the before times, more work is needed at the PR stage but by the time release is happening, you don't have to worry about this part. 🤷
Finalizing changelog for v4.3rc1
This PR is an attempt to make up the correct workflow to sync up the towncrier changes for 4.3.x with main (see #11847 as well).
I actually did the towncrier command in 4.3.x as part fo the release process (8ea8876) but then cherry-picked the SHA to make this PR. It seems to have gone smoothly - should I document this as the recommended workflow, @saimn @pllim @bsipocz @astrofrog?
My one question though, is: should we merge this now with the RC, or should we wait until the final 4.3 release happens? I'm not really clear on what we want to use as the workflow for anything that needs to be included between the RC and the real release... We can't use this exact same process because it would then have two separate sections, one for 4.3rc1 and another for 4.3. Does towncrier have a way to clean that up? Or should I just do a "manual" commit at 4.3 release that by-hand merges any post-RC changelog entries and updates the date/version name?