-
Notifications
You must be signed in to change notification settings - Fork 162
Changelog script: multiple changelog entries for a single PR #5279
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
base: master
Are you sure you want to change the base?
Changes from all commits
e914249
a89ca7d
8806969
c609105
90cfd0d
e78c6e5
cfe0d85
87a4b3e
becb7ea
d17b11a
9cc0082
263440c
fc2151b
494e65d
b9088d8
935c31e
5cca31e
50f238c
2a6a6d6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,7 +13,7 @@ jobs: | |
| steps: | ||
| - uses: yogevbd/[email protected] | ||
| with: | ||
| REQUIRED_LABELS_ANY: "release notes: added,release notes: highlight,release notes: not needed,release notes: to be added,release notes: use title" | ||
| REQUIRED_LABELS_ANY: "release notes: added,release notes: highlight,release notes: not needed,release notes: to be added,release notes: use title,release notes: use body" | ||
| REQUIRED_LABELS_ANY_DESCRIPTION: "Select at least one label concerning release notes" | ||
| BANNED_LABELS: "breaking,DO NOT MERGE,needs tests,WIP" | ||
| BANNED_LABELS_DESCRIPTION: "A PR should not be merged with `DO NOT MERGE`, `breaking`, `needs tests`, or `WIP` labels" | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -53,8 +53,63 @@ labelled. We have the following labels, along with how they are meant to be appl | |||||
| | `release notes: not needed` | This PR does not warrant an entry in the release notes. Internal only changes, like reorganization of private functions, changes to the test pipeline, etc can be tagged with this | | ||||||
| | `release notes: use title` | The release notes for this PR should be based on the title of this PR. The script will turn \$TITLE from the PR to `[#xyz] $TITLE` | | ||||||
| | `release notes: to be added` | These PRs will be marked by the script as a prominent TODO item. Check these PRs manually, and after updating them appropriately, relabel these items to either `release notes: added` or `release notes: use title` | | ||||||
| | `release notes: use body` | The changelog notes for this PR have been supplied in the body of the PR. Check [Release Notes: Use Body](#Release-Notes:-Use-Body) for documentation of the exact syntax. | | ||||||
| | none of the above | These PRs will be added to a separate prominent TODO category. Check these PRs manually, and after updating them appropriately, relabel these items to one of `release notes: added`, `release notes: use title`, or `release notes: not needed` | | ||||||
|
|
||||||
| #### Release Notes: Use Body | ||||||
|
|
||||||
| It is possible to manually supply release notes in the body of the PR (the first | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe the "body" is usually called "description"
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The API calls it "body". I can change it to "description" if that is more widely understood as "first comment of the PR"
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am fine with using "body" as long as this term is used consistently. We can simply explain it better and even link it to the term "description". For example:
|
||||||
| comment in the PR). To do this, make a section in your PR title by putting a | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. PR title?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be whatever is the first part called (body, or description, whatever the consensus is)
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wee it definitely shouldn't be "title", because that's simply wrong.
Suggested change
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, calling it title was a mistake. But I don't want to change it before we have a decision for whether to call it 'body', or, whether to call it 'description'. (or, there is potential for a change, and then another change, if we pick the incorrect alternative the first time)
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The label already uses |
||||||
| second level heading named `Release Notes` between two horizontal lines, then | ||||||
| adding release note entries as a list. This allows for having multiple entries | ||||||
| in the changelog for a single PR. It is possible to label each of the entries | ||||||
| with their own topic / pr type labels. | ||||||
|
|
||||||
| The syntax is the following : | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
|
|
||||||
| ```md | ||||||
| --- | ||||||
| ## Release Notes | ||||||
| - item1 {label1, label2} | ||||||
| - item2 {label3, label4, label5} | ||||||
| - item3 {label5} | ||||||
|
|
||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the empty line here required? If yes, please mention that. If not, can we remove it in all examples?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Strictly speaking, if you have 3 lines (
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could also simplify this by leaving out the
Then you can properly parse it but without regex, but please also with the current search hack, as follows
That is easy to explain, robust (no weird matches in the middle somewhere) and should be reasonably to use. |
||||||
| --- | ||||||
| ``` | ||||||
|
|
||||||
| As an example, the following body in the PR is rendered as follows : | ||||||
|
|
||||||
| ```md | ||||||
| --- | ||||||
| ## Release Notes | ||||||
| - Does abc {package: AbstractAlgebra, renaming} | ||||||
| - Does 123 {package: Nemo, documentation} | ||||||
| - Questions 42 {package: Singular, serialization} | ||||||
|
|
||||||
| --- | ||||||
| ``` | ||||||
|
|
||||||
| > | ||||||
| > ### Changes related to the package AbstractAlgebra | ||||||
| > | ||||||
| > #### Renamings | ||||||
| > | ||||||
| > - [#nnnn](https://github.com/oscar-system/Oscar.jl/pull/nnnn) Does abc | ||||||
| > | ||||||
| > ### Changes related to the package Nemo | ||||||
| > | ||||||
| > #### Improvements or additions to documentation | ||||||
| > | ||||||
| > - [#nnnn](https://github.com/oscar-system/Oscar.jl/pull/nnnn) Does 123 | ||||||
| > | ||||||
| > ### Changes related to the package Singular | ||||||
| > | ||||||
| > #### Changes related to serializing data in the MRDI file format | ||||||
| > | ||||||
| > - [#nnnn](https://github.com/oscar-system/Oscar.jl/pull/nnnn) Questions 42 | ||||||
| > | ||||||
|
|
||||||
|
|
||||||
| ### Secondary Labels: Topic | ||||||
|
|
||||||
| In addition to the release notes action labels, you can tag your PR with these following | ||||||
|
|
||||||
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.
There are a bunch of changes in this PR that are independent from the actual purpose of this PR, but each time I look at the python code, I have to filter them out mentally "ah yes I already looked at this, it does XYZ and is fine".
It would really help if that would go into another PR, or at least a separate commit, that only does that refactoring but nothing else.
Specifically
topicsandprtypesinto dictsstartdatetotimestampstartdateargumentprintstatementI think that would cut down the diff in half and could be merged right away.
Plus we'd be free to work on renaming the labels as @joschmitt suggested elsewhere in a separate PR (as this PR here would no longer touch the list of topics/prtypes)
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.
Sorry, this PR got out of hand.
Would you prefer :
or
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.
I think 1 would be easier