-
Notifications
You must be signed in to change notification settings - Fork 228
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
tools: Add ChangeLog helper script #2257
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When trying this out, I was in a working directory that had my own fork as the base repo for gh
(in .git/config
):
[remote "origin"]
url = [email protected]:softins/jamulus.git
fetch = +refs/heads/*:refs/remotes/origin/*
gh-resolved = base
That meant it couldn't view any of the upstream PRs:
tony@pi:~/jamulus $ gh pr list
There are no open pull requests in softins/jamulus
If I switched to my other working directory that only had jamulussoftware/jamulus
as a remote, gh pr
worked fine:
tony@pi:~/jamulus-upstream $ gh pr list
Showing 20 of 20 open pull requests in jamulussoftware/jamulus
......
One suggestion would be to replace gh
with $GH
everywhere, and then at the top do:
GH="gh -R jamulussoftware/jamulus"
It would then look at the upstream PRs whatever remote the user had set as the gh
base.
Alternatively, prominently document the need to set gh-resolved
to the upstream repo if running multiple remotes, and/or abort the script with an explanation if gh pr view N
returns non-zero.
Once I understood the problem, it was easy to change the gh
base in my normal working directory, to point to the upstream
repo by default.
if [[ ${title_suggestion_in_pr} == "SKIP" ]]; then | ||
continue | ||
fi | ||
fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there is no title suggestion, do you want to create a placeholder as a reminder that it needs filling in? Or to leave the PR so that find-missing-entries
still flags it up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hrm, I think it already should (unless I misunderstand)?
The current logic should be like this:
- If there is a
CHANGELOG:
line in the PR, it is interpreted. - If the line equals
SKIP
, the whole entry is skipped. There will be no placeholder either. This is intended for PRs like this one which do not necessarily have to be part of the ChangeLog. - If the line is not
SKIP
, it is used verbatim. - If there's not
CHANGELOG:
line, the PR title is used.
This probably implies that we could do us a favor by editing PR titles and/or adding CHANGELOG:
comments on merge.
A better way to do this would just be to set and export
Then there is no need for |
a1b17e4
to
5e77d90
Compare
Ah, didn't think of that, but that's certainly important to handle properly.
Sounds good, I've added that. |
59f32cf
to
e301d61
Compare
e301d61
to
fade6db
Compare
fade6db
to
6403eef
Compare
6403eef
to
35af730
Compare
35af730
to
3164f03
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
3164f03
to
827aaf0
Compare
Is this aiming for 3.9.0? Should it have a milestone set? |
)" | ||
else | ||
# Existing translation entries, so sort them: | ||
local changelog_before_translations="" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can sed edit "in-place" so we don‘t need to copy loads of data into a variable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sed
does support in-place editing, but I don't think those features are sufficient to do what's needed here. Feel free to suggest an alternative, that's what I came up with in absence of any better approaches.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe include a FIXME comment explaining that this could be improved
After thinking and looking at the past change logs, it seems as if we got shorter in our description style of changes. Is this good or bad? Line 62 in 8bbff18
for example is not that verbose Lines 260 to 263 in 8bbff18
For example gives a lot more detail. We could still give users a little advice how to use the new feature (and not just give a commit-message style entry). |
The change log entries shouldn't be limited to a single line, for sure. In my view, they shouldn't, however, attempt to explain the problem and what was done to solve it -- only to say what has improved and link to the full description (i.e. issue, if available, and pull request). So anything saying "Improve " is a bit redundant :).
could be
As it covers all cases (GUI, non-GUI, client, server) adding a limiting clause is redundant. However, "more detail" doesn't really say anything, either... What changed? :) I don't think "contributed by" needs saying -- the format is condensed: the text in brackets at the end is a list (separated by commas, no "and") of contributors. If that needs explaining, say it once. So what I want:
|
I really like the tag-style as it makes it possible to group entries and to enable Changelog skimming. As a Linux user I could skip all Windows entries. I agree thought hat there are types of changes which are hard to tag, so there may be exemptions.
I think we haven't been consistenly adding the issue numbers as well. The changelog helper currently doesn't (but it probably could when the Github metadata is up-to-date (
I've just taken the style which has been there (I think it moved from In general, I'd rather like to get this PR merged at some point. It's been sitting around for quite some time, yet it is the tool I've been using to generate all the recent changelog PRs. We can always iterate on further improvement ideas later. |
The helper verifies that the ChangeLog file contains references to all PRs from `git log` and to all PRs from the relevant Github milestone. It can also automatically generate entries for PRs. It respects the magic `CHANGELOG:` keyword in PRs and PR comments to allow users to help fill this. It respects the magic `CHANGELOG: SKIP` term to avoid a ChangeLog entry for a given PR. It supports grouping and sorting within the groups. Relevant discussion: jamulussoftware#1839 Co-authored-by: ann0see <[email protected]>
Relevant discussion: jamulussoftware#1839 Co-authored-by: ann0see <[email protected]>
827aaf0
to
7593156
Compare
Yes, by "Section" I mean "UI:" or whatever. But "UI, Command Line, Client, Server, Windows, Linux, MacOS, Android, iOS:" seems a bit unhelpful... it's really helpful if the change only affects one area and becomes less helpful the more areas the change covers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going to approve this now.
)" | ||
else | ||
# Existing translation entries, so sort them: | ||
local changelog_before_translations="" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe include a FIXME comment explaining that this could be improved
contribute/en/Release-Process: Document changelog helper jamulussoftware/jamulus#2257
Short description of changes
This PR adds release tooling to validate the ChangeLog for completeness.
git log
.Context: Fixes an issue?
Does this change need documentation? What needs to be documented and how?
CHANGELOG:
syntax should be suggested in the PR template. This is done as part of this PR.Status of this Pull Request
Works, but open to improvements.
What is missing until this pull request can be merged?
Reviews.
Checklist
My code follows the style guide(no bash style guide)Originally posted by @softins in #2249 (comment)
It's listed because it's part of the history in master (see a8853b6). Might be a stray reference.
The tool uses Github PRs and their milestone to find relevant PRs. However, we might have forgotten to assign a milestone. That's why the script also scans
git log
and uses the references there as well.Finally, let this PR be handled properly in the tool...:
CHANGELOG: SKIP