Skip to content
Open
Show file tree
Hide file tree
Changes from 15 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/EnforceLabels.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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"
166 changes: 110 additions & 56 deletions dev/releases/release_notes.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@

import json
import os
import re
import copy
import subprocess
import sys
from datetime import datetime
Expand Down Expand Up @@ -84,38 +86,38 @@ def warning(s):
# the given label is put into the corresponding section; each PR is put into only one section, the first one
# one from this list it fits in.
# See also <https://github.com/gap-system/gap/issues/4257>.
topics = [
Copy link
Member

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

  • changing topics and prtypes into dicts
  • renaming startdate to timestamp
  • dropping an unused startdate argument
  • removing a print statement

I 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)

Copy link
Member Author

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 :

  1. Splitting out this into 2 PRs, one which just deals with the multiple entries for a single PR, and another which deals with other things

or

  1. Rewriting some history, to merge all the "patchwork" commits into one commit, and leave the commits which actually tackle the topic at hand separate (I think this might be more involved?)

Copy link
Member

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

["release notes: highlight", "Highlights"],
["topic: algebraic geometry", "Algebraic Geometry"],
["topic: combinatorics", "Combinatorics"],
["topic: commutative algebra", "Commutative Algebra"],
["topic: FTheoryTools", "F-Theory Tools"],
["topic: groups", "Groups"],
["topic: lie theory", "Lie Theory"],
["topic: number theory", "Number Theory"],
["topic: polyhedral geometry", "Polyhedral Geometry"],
["topic: toric geometry", "Toric Geometry"],
["topic: tropical geometry", "Tropical Geometry"],
["package: AbstractAlgebra", "Changes related to the package AbstractAlgebra"],
["package: AlgebraicSolving", "Changes related to the package AlgebraicSolving"],
["package: GAP", "Changes related to the package GAP"],
["package: Hecke", "Changes related to the package Hecke"],
["package: Nemo", "Changes related to the package Nemo"],
["package: Polymake", "Changes related to the package Polymake"],
["package: Singular", "Changes related to the package Singular"],
]
prtypes = [
["renaming", "Renamings"],
["serialization", "Changes related to serializing data in the MRDI file format"],
["enhancement", "New features or extended functionality"],
["experimental", "Only changes experimental parts of OSCAR"],
["optimization", "Performance improvements or improved testing"],
["bug: wrong result", "Fixed bugs that returned incorrect results"],
["bug: crash", "Fixed bugs that could lead to crashes"],
["bug: unexpected error", "Fixed bugs that resulted in unexpected errors"],
["bug", "Other fixed bugs"],
["documentation", "Improvements or additions to documentation"],
]
topics = {
"release notes: highlight": "Highlights",
"topic: algebraic geometry": "Algebraic Geometry",
"topic: combinatorics": "Combinatorics",
"topic: commutative algebra": "Commutative Algebra",
"topic: FTheoryTools": "F-Theory Tools",
"topic: groups": "Groups",
"topic: lie theory": "Lie Theory",
"topic: number theory": "Number Theory",
"topic: polyhedral geometry": "Polyhedral Geometry",
"topic: toric geometry": "Toric Geometry",
"topic: tropical geometry": "Tropical Geometry",
"package: AbstractAlgebra": "Changes related to the package AbstractAlgebra",
"package: AlgebraicSolving": "Changes related to the package AlgebraicSolving",
"package: GAP": "Changes related to the package GAP",
"package: Hecke": "Changes related to the package Hecke",
"package: Nemo": "Changes related to the package Nemo",
"package: Polymake": "Changes related to the package Polymake",
"package: Singular": "Changes related to the package Singular",
}
prtypes = {
"renaming": "Renamings",
"serialization": "Changes related to serializing data in the MRDI file format",
"enhancement": "New features or extended functionality",
"experimental": "Only changes experimental parts of OSCAR",
"optimization": "Performance improvements or improved testing",
"bug: wrong result": "Fixed bugs that returned incorrect results",
"bug: crash": "Fixed bugs that could lead to crashes",
"bug: unexpected error": "Fixed bugs that resulted in unexpected errors",
"bug": "Other fixed bugs",
"documentation": "Improvements or additions to documentation",
}


def get_tag_date(tag: str) -> str:
Expand Down Expand Up @@ -149,7 +151,7 @@ def get_pr_list(date: str, extra: str) -> List[Dict[str, Any]]:
"--search",
query,
"--json",
"number,title,closedAt,labels,mergedAt",
"number,title,closedAt,labels,mergedAt,body",
"--limit",
"200",
],
Expand All @@ -165,16 +167,33 @@ def get_pr_list(date: str, extra: str) -> List[Dict[str, Any]]:
def pr_to_md(pr: Dict[str, Any]) -> str:
"""Returns markdown string for the PR entry"""
k = pr["number"]
title = pr["title"]
return f"- [#{k}](https://github.com/oscar-system/Oscar.jl/pull/{k}) {title}\n"
if has_label(pr, 'release notes: use body'):
mdstring = pr['body'].replace("- ", f"- [#{k}](https://github.com/oscar-system/Oscar.jl/pull/{k}) ")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't this also replace any - that occurs in the middle of a description? Might be better to use a regex anchor, i.e., something like

Suggested change
mdstring = pr['body'].replace("- ", f"- [#{k}](https://github.com/oscar-system/Oscar.jl/pull/{k}) ")
mdstring = pr['body'].replace(r"^- ", f"- [#{k}](https://github.com/oscar-system/Oscar.jl/pull/{k}) ")

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know why, but as of this moment, I am unwilling to use regx. I have tried to make an alternate solution: replace only the first occurrence in string.

I think regx (dash only at first position) might be a stronger solution, but my most recent changes might be enough in this case.

I will change it to regx if you think its better in any case.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, there is a concrete bug that can relatively easily occur with your current code, so yeah, I really think you should either use the regex here, or use some other approach that ensures the dash is at the start of a line (e.g. you could also match for "\n- " and replace by "\n- ...")

else:
title = pr["title"]
mdstring = f"- [#{k}](https://github.com/oscar-system/Oscar.jl/pull/{k}) {title}\n"
return mdstring

def body_to_release_notes(pr):
body = pr['body']
index1 = body.lower().find("## release notes")
if index1 == -1:
## not found
## complain and return fallback
print(f"Release notes section not found in PR number {pr['number']}!!")
return body
index2 = body.find('---', index1)
# there are 17 characters from index 1 until the next line
mdstring = body[index1+17:index2]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But what if there is a line ## release notes follow below instead? Or if the trailing --- is missing? All of these can easily happen by accident.

How about instead using a regex like this (untested, just meant as a rough guide), and then extracting the correcting matching group. (Of course multiline regex matching must be enabled for this, however one does that in Python)

r"^## release notes\n(.+)^---\n"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the same strange reason as above ("gut feeling"), I tried to solve this problem without using regx.

I will change it to the regx solution if you think it is a better solution than my latest commits

return mdstring


def has_label(pr: Dict[str, Any], label: str) -> bool:
return any(x["name"] == label for x in pr["labels"])


def changes_overview(
prs: List[Dict[str, Any]], startdate: str, new_version: str
prs: List[Dict[str, Any]], timestamp: str, new_version: str
) -> None:
"""Writes files with information for release notes."""

Expand Down Expand Up @@ -209,32 +228,35 @@ def changes_overview(
countedPRs = 0
for priorityobject in topics:
matches = [
pr for pr in prs_with_use_title if has_label(pr, priorityobject[0])
pr for pr in prs_with_use_title if has_label(pr, priorityobject)
]
print("PRs with label '" + priorityobject[0] + "': ", len(matches))
print("PRs with label '" + priorityobject + "': ", len(matches))
print(matches)
countedPRs = countedPRs + len(matches)
if len(matches) == 0:
continue
relnotes_file.write("### " + priorityobject[1] + "\n\n")
if priorityobject[1] == 'Highlights':
relnotes_file.write("### " + topics[priorityobject] + "\n\n")
if topics[priorityobject] == 'Highlights':
itervar = topics
else:
itervar = prtypes
for typeobject in itervar:
if typeobject[1] == priorityobject[1]:
if typeobject == priorityobject:
continue
matches_type = [
pr for pr in matches if has_label(pr, typeobject[0])
pr for pr in matches if has_label(pr, typeobject)
]
print("PRs with label '" + priorityobject[0] + "' and type '" + typeobject[0] + "': ", len(matches_type))
print("PRs with label '" + priorityobject + "' and type '" + typeobject + "': ", len(matches_type))
if len(matches_type) == 0:
continue
relnotes_file.write(f"#### {typeobject[1]}\n\n")
relnotes_file.write(f"#### {itervar[typeobject]}\n\n")
remove_from_matches = []
for pr in matches_type:
relnotes_file.write(pr_to_md(pr))
prs_with_use_title.remove(pr)
matches.remove(pr)
remove_from_matches.append(pr)
for pr in remove_from_matches:
matches_type.remove(pr)
relnotes_file.write('\n')
print(f"Remaining PRs: {totalPRs - countedPRs}")
Expand All @@ -244,15 +266,14 @@ def changes_overview(
if len(prs_with_use_title) > 0:
relnotes_file.write("### Other changes\n\n")
for typeobject in prtypes:
print(typeobject)
matches_type = [
pr for pr in prs_with_use_title if has_label(pr, typeobject[0])
pr for pr in prs_with_use_title if has_label(pr, typeobject)
]
len(matches_type)
print("PRs with label '" + priorityobject[0] + "' and type '" + typeobject[0] + "': ", len(matches_type))
print("PRs with type '" + typeobject + "': ", len(matches_type))
if len(matches_type) == 0:
continue
relnotes_file.write("#### " + typeobject[1] + "\n\n")
relnotes_file.write("#### " + prtypes[typeobject] + "\n\n")

for pr in matches_type:
relnotes_file.write(pr_to_md(pr))
Expand Down Expand Up @@ -280,10 +301,10 @@ def changes_overview(
"general section of the topic section in the changelog.\n\n")
for pr in prs_with_use_title:
for topic in topics:
matches = [pr for pr in prs_with_use_title if has_label(pr, topic[0])]
matches = [pr for pr in prs_with_use_title if has_label(pr, topic)]
if len(matches) == 0:
continue
relnotes_file.write(f'#### {topic[1]}\n\n')
relnotes_file.write(f'#### {topics[topic]}\n\n')
for match in matches:
relnotes_file.write(pr_to_md(match))
prs_with_use_title.remove(match)
Expand Down Expand Up @@ -316,6 +337,40 @@ def changes_overview(
# finally copy over this new file to changelog.md
os.rename(newfile, finalfile)

def split_pr_into_changelog(prs: List):
childprlist = []
toremovelist = []
for pr in prs:
if has_label(pr, 'release notes: use body'):
mdstring = body_to_release_notes(pr).strip()
mdlines = mdstring.split('\r\n')
pattern = r'\{.*\}$'
for line in mdlines:
mans = re.search(pattern, line)
if not mans:
# maybe actually raise an error instead of just continue
warning(f"PR number #{pr['number']} is tagged as \"Use Body\", but the body "
"does not provide tags! This will result in TODO changelog items!")
continue
label_list = mans.group().strip('{').strip('}').split(',')
cpr = copy.deepcopy(pr)
for label in label_list:
label = label.strip()
if not (label in prtypes or label in topics):
warning(f"PR number #{pr['number']}'s changelog body has label {label}, "
"which is not a label we recognize ! We are ignoring this label. "
"This might result in a TODO changelog item!")
continue
cpr['labels'].append({'name': label})
mindex = mans.span()[0]
line = line[0:mindex]
cpr['body'] = f'{line.strip()}\n'
childprlist.append(cpr)
if pr not in toremovelist:
toremovelist.append(pr)
prs.extend(childprlist)
prlist = [pr for pr in prs if pr not in toremovelist]
return prlist

def main(new_version: str) -> None:
major, minor, patchlevel = map(int, new_version.split("."))
Expand All @@ -339,7 +394,7 @@ def main(new_version: str) -> None:
extra = f'label:"backport {major}.{minor}.x done"'

if release_type == 2:
startdate = get_tag_date(basetag)
timestamp = get_tag_date(basetag)
else:
# Find the timestamp of the last shared commit
shared_commit = subprocess.run([
Expand All @@ -355,20 +410,19 @@ def main(new_version: str) -> None:
"--format=\"%cI\"",
shared_commit
], shell=False, check=True, capture_output=True).stdout.decode().strip().replace('"', '')
# date is first 10 characters of timestamp
startdate = timestamp[0:10]
print("Base tag is", basetag)
print("Last common commit at ", startdate)
print("Last common commit at ", timestamp)

print("Downloading filtered PR list")
prs = get_pr_list(startdate, extra)
prs = get_pr_list(timestamp, extra)
prs = split_pr_into_changelog(prs)
# print(json.dumps(prs, sort_keys=True, indent=4))

# reset changelog file to state tracked in git

subprocess.run(f'git checkout -- {finalfile}'.split(), check=True)

changes_overview(prs, startdate, new_version)
changes_overview(prs, timestamp, new_version)


if __name__ == "__main__":
Expand Down
55 changes: 55 additions & 0 deletions docs/src/DeveloperDocumentation/changelog.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the "body" is usually called "description"

Copy link
Member Author

Choose a reason for hiding this comment

The 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"

Copy link
Member

Choose a reason for hiding this comment

The 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:

(the body of the PR is the first comment in the PR, created at the same time as the PR is created; it is also sometimes call the "description" of the PR).

comment in the PR). To do this, make a section in your PR title by putting a
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR title?

Copy link
Member Author

Choose a reason for hiding this comment

The 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)

Copy link
Member

Choose a reason for hiding this comment

The 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
comment in the PR). To do this, make a section in your PR title by putting a
comment in the PR). To do this, make a section in your PR body by putting a

Copy link
Member Author

Choose a reason for hiding this comment

The 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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The label already uses body, the API uses body, and my code change suggestions here everywhere use body, so we're using body :-)

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 :
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
The syntax is the following :
The syntax is the following:


```md
---
## Release Notes
- item1 {label1, label2}
- item2 {label3, label4, label5}
- item3 {label5}

Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strictly speaking, if you have 3 lines ( --- ) right after a line, that is, no empty line, then it is treated as a heading in markdown. It doesn't make a difference in our script, but not leaving that last empty line makes the markdown syntax highlighting color the last line differently, possible confusing someone?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also simplify this by leaving out the --- bits completely. Just say:

  • it starts on a line that is exactly ## Release Notes (I'd be OK with ignoring trailing spaces as those sometimes sneak in and are "invisible"; and I'd also be OK with ignoring case)
  • then we expects lines starting with - and of the form - description {label1, label2}
  • we ignore any empty (except for whitespace) lines before, between or after
  • as soon as we hit a line that is neither empty nor starts with - we stop the scanning.

Then you can properly parse it but without regex, but please also with the current search hack, as follows

  1. split the body into lines, rstrip each line (to remove trailing whitespace)
  2. scan the array of line until you find one matching ## Release Notes (ignoring case)
  3. then iterate over the following lines and..
    • skip empty ones
    • parse those starting with -
    • abort the search if you find anything else

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
Expand Down
Loading