Conversation
psss
force-pushed
the
psss-sprint-overview
branch
from
fcae090 to
32d12ce
Compare
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
Added a helper script showing all issues and pull requests included in the given sprint. Provides a human readable text overview and yaml export for further processing. This is the first half of the jira syncing automation. The second part will be completed once the Jira migration is done. Fix #4604. Assisted-by: Claude
psss
force-pushed
the
psss-sprint-overview
branch
from
32d12ce to
97ac27a
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
psss
added
the
area | automation
|
/gemini review |
|
Example output: |
There was a problem hiding this comment.
Code Review
This pull request introduces a new script, sprint-overview, to provide an overview of issues and pull requests for a given sprint. The script is well-structured and provides both human-readable and YAML output formats. I've identified a potential security/correctness issue related to handling sprint names with special characters, and a couple of areas where the code can be refactored for better readability and maintainability.
| "orgName": org, | ||
| "projectNumber": project, | ||
| "cursor": cursor, | ||
| "query": f'Sprint:"{sprint_name}"', |
There was a problem hiding this comment.
The sprint_name is used directly in the GraphQL query string. If the sprint name contains special characters, such as a double quote, it could break the query or lead to incorrect results. Sanitize the sprint name by escaping double quotes before incorporating it into the query string.
| "query": f'Sprint:"{sprint_name}"', | |
| "query": f'Sprint:"{sprint_name.replace('"', '\\"')}"', |
| key = ( | ||
| "open_prs" | ||
| if state == "OPEN" | ||
| else "merged_prs" | ||
| if state == "MERGED" | ||
| else "closed_prs" | ||
| ) |
There was a problem hiding this comment.
This nested ternary operator is difficult to read. Replace it with a standard if/elif/else block to improve code clarity and maintainability.
if state == "OPEN":
key = "open_prs"
elif state == "MERGED":
key = "merged_prs"
else:
key = "closed_prs"| output = { | ||
| "sprint": sprint_name, | ||
| "total": { | ||
| "items": total, | ||
| "story_points": int(sum(total_points.values())), | ||
| }, | ||
| "categories": { | ||
| "open_issues": { | ||
| "count": len(categories["open_issues"]), | ||
| "story_points": total_points["open_issues"], | ||
| "items": [ | ||
| { | ||
| "url": url, | ||
| "title": title, | ||
| "story_points": int(size) if size is not None else None, | ||
| } | ||
| for _, title, size, url in categories["open_issues"] | ||
| ], | ||
| }, | ||
| "open_prs": { | ||
| "count": len(categories["open_prs"]), | ||
| "story_points": total_points["open_prs"], | ||
| "items": [ | ||
| { | ||
| "url": url, | ||
| "title": title, | ||
| "story_points": int(size) if size is not None else None, | ||
| } | ||
| for _, title, size, url in categories["open_prs"] | ||
| ], | ||
| }, | ||
| "closed_issues": { | ||
| "count": len(categories["closed_issues"]), | ||
| "story_points": total_points["closed_issues"], | ||
| "items": [ | ||
| { | ||
| "url": url, | ||
| "title": title, | ||
| "story_points": int(size) if size is not None else None, | ||
| } | ||
| for _, title, size, url in categories["closed_issues"] | ||
| ], | ||
| }, | ||
| "merged_prs": { | ||
| "count": len(categories["merged_prs"]), | ||
| "story_points": total_points["merged_prs"], | ||
| "items": [ | ||
| { | ||
| "url": url, | ||
| "title": title, | ||
| "story_points": int(size) if size is not None else None, | ||
| } | ||
| for _, title, size, url in categories["merged_prs"] | ||
| ], | ||
| }, | ||
| "closed_prs": { | ||
| "count": len(categories["closed_prs"]), | ||
| "story_points": total_points["closed_prs"], | ||
| "items": [ | ||
| { | ||
| "url": url, | ||
| "title": title, | ||
| "story_points": int(size) if size is not None else None, | ||
| } | ||
| for _, title, size, url in categories["closed_prs"] | ||
| ], | ||
| }, | ||
| }, | ||
| "summary": { | ||
| "open": {"count": open_count, "story_points": int(open_points)}, | ||
| "closed": {"count": closed_count, "story_points": int(closed_points)}, | ||
| }, | ||
| } |
There was a problem hiding this comment.
The logic for building each category in the YAML output is duplicated. Refactor this to follow the Don't Repeat Yourself (DRY) principle, which will make the code more maintainable. You can use a dictionary comprehension to generate the categories dictionary.
For example:
category_keys = (
"open_issues",
"open_prs",
"closed_issues",
"merged_prs",
"closed_prs",
)
output_categories = {
key: {
"count": len(categories[key]),
"story_points": total_points[key],
"items": [
{
"url": url,
"title": title,
"story_points": int(size) if size is not None else None,
}
for _, title, size, url in categories[key]
],
}
for key in category_keys
}
output = {
"sprint": sprint_name,
"total": {
"items": total,
"story_points": int(sum(total_points.values())),
},
"categories": output_categories,
"summary": {
"open": {"count": open_count, "story_points": int(open_points)},
"closed": {"count": closed_count, "story_points": int(closed_points)},
},
}
| token = os.environ.get("GITHUB_PERSONAL_ACCESS_TOKEN") | ||
| if not token: | ||
| print("Error: GITHUB_PERSONAL_ACCESS_TOKEN environment variable not set", file=sys.stderr) | ||
| sys.exit(1) |
There was a problem hiding this comment.
Unless we are doing write requests, we do not need a GITHUB_PERSONAL_ACCESS_TOKEN. From what I see, this is doing only reads on PRs, so I think it could be refactored to use the rest api as read-only instead
There was a problem hiding this comment.
Actually, without the token, we're not able to access the project data. I had to explicitly allow the read:project permission for that token.
There was a problem hiding this comment.
Really? I can see them just fine from an incognito window. Does read:project restrict all reads or only reads to private repo/projects (the same like read:repository is unnecessary actually because of this).
If we do indeed need it, there should be a better way around this with github apps. Personal access tokens are a pain to manage, and would really like to avoid that hard dependency
| @@ -0,0 +1,403 @@ | |||
| #!/usr/bin/python3 | |||
There was a problem hiding this comment.
Could you include the PEP723 header like
| #!/usr/bin/python3 | |
| #!/usr/bin/python3 | |
| # /// script | |
| # dependencies = [ | |
| # "click", | |
| # "requests", | |
| # "ruamel.yaml", | |
| # ] | |
| # /// |
| Usage: | ||
| sprint-overview "Sprint 11" | ||
| sprint-overview "Sprint 11" --org myorg --project 2 | ||
| sprint-overview "Sprint 11" --yaml |
There was a problem hiding this comment.
Usage is already defined in the help message, let's not introduce a source of divergence
scripts/sprint-overview
Outdated
| parser.add_argument("sprint_name", help="Name of the sprint (e.g., 'Sprint 11')") | ||
| parser.add_argument( | ||
| "--org", default="teemtee", help="GitHub organization name (default: teemtee)" | ||
| ) | ||
| parser.add_argument("--project", type=int, default=1, help="Project number (default: 1)") | ||
| parser.add_argument( | ||
| "--yaml", | ||
| action="store_true", | ||
| help="Output in YAML format for machine-readable processing", | ||
| ) | ||
|
|
||
| args = parser.parse_args() |
There was a problem hiding this comment.
Could we not just hard-code these? Maybe at most having either yaml format or the format to be used in the slack messages.
| print(f"Sprint: {sprint_name}") | ||
| print(f"Total items: {total}") | ||
| print(f"Total story points: {sum(total_points.values()):.0f}") |
There was a problem hiding this comment.
I don't like the format being defined by all of these print commands. Can we have at least an f-string of the overall format?
There was a problem hiding this comment.
Or a template. Like Jinja. Exactly the task for which templating engines were invented...
I don't understand why people insist on building complex, multiline, structured text output with raw print() calls... It's like there's some bonus for suffering. Yeah, let me compete in the table tennis tournament, but I insist on having both arms tied behind my back, holding paddle in my mouth!
There was a problem hiding this comment.
And the whole concept of separating logic from the presentation layer. Interweaving both together makes maintenance harder.
| query = """ | ||
| query($orgName: String!, $projectNumber: Int!, $cursor: String, $query: String!) { | ||
| organization(login: $orgName) { | ||
| projectV2(number: $projectNumber) { | ||
| items(first: 100, after: $cursor, query: $query) { | ||
| nodes { | ||
| content { | ||
| ... on Issue { | ||
| __typename | ||
| number | ||
| title | ||
| state | ||
| url | ||
| } | ||
| ... on PullRequest { | ||
| __typename | ||
| number | ||
| title | ||
| state | ||
| url | ||
| } | ||
| } | ||
| size: fieldValueByName(name: "Size") { | ||
| ... on ProjectV2ItemFieldNumberValue { | ||
| number | ||
| } | ||
| } | ||
| } | ||
| pageInfo { | ||
| hasNextPage | ||
| endCursor | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| """ |
There was a problem hiding this comment.
Uph, isn't this super dark-magic for us? At least with the REST API we can point to the documentation of what is being requested: https://docs.github.com/en/rest/projects/items?apiVersion=2022-11-28#get-an-item-for-an-organization-owned-project
thrix
left a comment
thrix
left a comment
There was a problem hiding this comment.
Code Review: Implement the sprint-overview script
Reviewed against commit 7d37dae.
Summary
A helper script that fetches sprint items from GitHub Projects via GraphQL and displays them in human-readable text or YAML format. Well-structured with pagination support. There's extensive existing review feedback from @LecrisUT, @happz, Gemini, and Graphite — I'll focus on items not yet covered.
Issues
1. YAML output omits the issue/PR number (minor)
Each YAML item has url, title, story_points but not number. The text output prominently shows #4672, but the YAML output (intended for machine-readable processing) requires URL parsing to get the number. Since number is already available in the tuple, it should be included. See inline comment.
2. Silent skip of DraftIssues could be confusing (minor)
if item.get("content"): silently skips draft issues (which have no content type in GitHub Projects). A comment explaining this would help. See inline comment.
Agreement with existing feedback
The following open review comments from other reviewers are valid and worth addressing:
- @LecrisUT: PEP723 header — Adding dependency metadata (
click,requests,pyyaml) enablesuv run scripts/sprint-overviewwithout manual dependency management. - @LecrisUT/@happz: Output formatting — The
print()-based formatting is verbose and tightly couples logic with presentation. A template (Jinja or even a simple f-string template) would be cleaner and easier to maintain. - @gemini: DRY for YAML output — The 5x repeated category block in
display_items_yamlshould use a loop/comprehension. - @LecrisUT: GraphQL vs REST API — Worth investigating if the REST API can access project items without a token, reducing the setup barrier.
Verdict
Functional and useful script. The existing review feedback covers the main improvement areas well. The missing number in YAML output (issue #1) is the main new finding.
Generated-by: Claude Code
| "title": title, | ||
| "story_points": int(size) if size is not None else None, | ||
| } | ||
| for _, title, size, url in categories["open_issues"] |
There was a problem hiding this comment.
The number (issue/PR number) is discarded here with _, so YAML items only have url, title, story_points. The text output prominently shows #number, but the YAML output (intended for machine-readable processing per the --yaml help text) requires URL parsing to extract it. Since number is already in the tuple, consider including it:
{
"number": number,
"url": url,
"title": title,
"story_points": int(size) if size is not None else None,
}
for number, title, size, url in categories["open_issues"]Same applies to the other 4 category blocks below.
Generated-by: Claude Code
| print(f" Page {page}: {len(items)} items", file=sys.stderr) | ||
|
|
||
| for item in items: | ||
| if item.get("content"): |
There was a problem hiding this comment.
This silently skips draft issues (which have no content in GitHub Projects). A brief comment explaining the intent would help — e.g. # Skip draft issues which have no content type.
Generated-by: Claude Code
5 participants
Added a helper script showing all issues and pull requests included in the given sprint. Provides a human readable text overview and yaml export for further processing. This is the first half of the jira syncing automation. The second part will be completed once the Jira migration is done.
Fix #4604.
Pull Request Checklist