-
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 4 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,6 +13,8 @@ | |
|
|
||
| import json | ||
| import os | ||
| import re | ||
| import copy | ||
| import subprocess | ||
| import sys | ||
| from datetime import datetime | ||
|
|
@@ -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 = [ | ||
| ["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: | ||
|
|
@@ -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", | ||
| ], | ||
|
|
@@ -166,7 +168,24 @@ 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" | ||
| mdstring = f"- [#{k}](https://github.com/oscar-system/Oscar.jl/pull/{k}) {title}\n" | ||
| if has_label(pr,'release notes: use body'): | ||
| mdstring = body_to_release_notes(pr) | ||
| mdstring = mdstring.replace("- ", f"- [#{k}](https://github.com/oscar-system/Oscar.jl/pull/{k}) ") | ||
aaruni96 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| 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 mdstring | ||
| index2 = body.find('---', index1) | ||
| # there are 17 characters from index 1 until the next line | ||
| mdstring = body[index1+17:index2] | ||
|
||
| return mdstring | ||
|
|
||
|
|
||
| def has_label(pr: Dict[str, Any], label: str) -> bool: | ||
|
|
@@ -209,28 +228,28 @@ 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") | ||
| for pr in matches_type: | ||
| relnotes_file.write(pr_to_md(pr)) | ||
| prs_with_use_title.remove(pr) | ||
|
|
@@ -244,15 +263,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 label '" + priorityobject + "' and 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)) | ||
|
|
@@ -280,10 +298,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) | ||
|
|
@@ -316,6 +334,27 @@ def changes_overview( | |
| # finally copy over this new file to changelog.md | ||
| os.rename(newfile, finalfile) | ||
|
|
||
| def split_pr_into_changelog(prs: List): | ||
| childprlist = [] | ||
| 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'\{package: .*\}' | ||
aaruni96 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| for line in mdlines: | ||
| if not '{package: ' in line: | ||
| continue | ||
| mans = re.search(pattern, line) | ||
aaruni96 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| packagestring = mans.group()[1:-1] | ||
| cpr = copy.deepcopy(pr) | ||
| mindex = line.find('{package:') | ||
| line = line[0:mindex] | ||
| cpr['labels'].append({'name': packagestring}) | ||
| cpr['body'] = f'---\r\n## Release Notes\r\n{line}\r\n---' | ||
| childprlist.append(cpr) | ||
| prs.remove(pr) | ||
aaruni96 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| prs.extend(childprlist) | ||
| return prs | ||
|
|
||
| def main(new_version: str) -> None: | ||
| major, minor, patchlevel = map(int, new_version.split(".")) | ||
|
|
@@ -362,6 +401,7 @@ def main(new_version: str) -> None: | |
|
|
||
| print("Downloading filtered PR list") | ||
| prs = get_pr_list(startdate, extra) | ||
| prs = split_pr_into_changelog(prs) | ||
| # print(json.dumps(prs, sort_keys=True, indent=4)) | ||
|
|
||
| # reset changelog file to state tracked in git | ||
|
|
||
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