From: Simon Glass <simon.glass@canonical.com> Change get_pickman_mrs() and get_mr_comments() to return lists of named tuples instead of dicts. This provides cleaner attribute access (e.g. merge_req.iid instead of merge_req['iid']) and better code documentation. Add PickmanMr and MrComment named tuples to gitlab_api.py. Co-developed-by: Claude Opus 4.5 <noreply@anthropic.com> Signed-off-by: Simon Glass <simon.glass@canonical.com> --- tools/pickman/agent.py | 2 +- tools/pickman/control.py | 24 +++++++++---------- tools/pickman/ftest.py | 24 +++++++++++-------- tools/pickman/gitlab_api.py | 48 ++++++++++++++++++++++--------------- 4 files changed, 56 insertions(+), 42 deletions(-) diff --git a/tools/pickman/agent.py b/tools/pickman/agent.py index 176cf9773a0..932d61be4d7 100644 --- a/tools/pickman/agent.py +++ b/tools/pickman/agent.py @@ -154,7 +154,7 @@ async def run_review_agent(mr_iid, branch_name, comments, remote, repo_path=None # Format comments for the prompt comment_text = '\n'.join( - f"- [{c['author']}]: {c['body']}" + f'- [{c.author}]: {c.body}' for c in comments ) diff --git a/tools/pickman/control.py b/tools/pickman/control.py index 973db22e106..09ffa4a3da3 100644 --- a/tools/pickman/control.py +++ b/tools/pickman/control.py @@ -528,29 +528,29 @@ def process_mr_reviews(remote, mrs): processed = 0 for merge_req in mrs: - comments = gitlab_api.get_mr_comments(remote, merge_req['iid']) + comments = gitlab_api.get_mr_comments(remote, merge_req.iid) if comments is None: continue # Filter to unresolved comments - unresolved = [c for c in comments if not c.get('resolved', True)] + unresolved = [c for c in comments if not c.resolved] if not unresolved: continue tout.info('') - tout.info(f"MR !{merge_req['iid']} has {len(unresolved)} comment(s):") + tout.info(f"MR !{merge_req.iid} has {len(unresolved)} comment(s):") for comment in unresolved: - tout.info(f" [{comment['author']}]: {comment['body'][:80]}...") + tout.info(f' [{comment.author}]: {comment.body[:80]}...') # Run agent to handle comments success, _ = agent.handle_mr_comments( - merge_req['iid'], - merge_req['source_branch'], + merge_req.iid, + merge_req.source_branch, unresolved, remote, ) if not success: - tout.error(f"Failed to handle comments for MR !{merge_req['iid']}") + tout.error(f"Failed to handle comments for MR !{merge_req.iid}") processed += 1 return processed @@ -582,7 +582,7 @@ def do_review(args, dbs): # pylint: disable=unused-argument tout.info(f'Found {len(mrs)} open pickman MR(s):') for merge_req in mrs: - tout.info(f" !{merge_req['iid']}: {merge_req['title']}") + tout.info(f" !{merge_req.iid}: {merge_req.title}") process_mr_reviews(remote, mrs) @@ -632,7 +632,7 @@ def process_merged_mrs(remote, source, dbs): processed = 0 for merge_req in merged_mrs: - mr_source, last_hash = parse_mr_description(merge_req['description']) + mr_source, last_hash = parse_mr_description(merge_req.description) # Only process MRs for the requested source branch if mr_source != source: @@ -648,7 +648,7 @@ def process_merged_mrs(remote, source, dbs): full_hash = run_git(['rev-parse', last_hash]) except Exception: # pylint: disable=broad-except tout.warning(f"Could not resolve commit '{last_hash}' from " - f"MR !{merge_req['iid']}") + f"MR !{merge_req.iid}") continue # Check if this commit is an ancestor of source but not of current @@ -669,7 +669,7 @@ def process_merged_mrs(remote, source, dbs): # Update database short_old = current[:12] short_new = full_hash[:12] - tout.info(f"MR !{merge_req['iid']} merged, updating '{source}': " + tout.info(f"MR !{merge_req.iid} merged, updating '{source}': " f'{short_old} -> {short_new}') dbs.source_set(source, full_hash) dbs.commit() @@ -708,7 +708,7 @@ def do_step(args, dbs): if mrs: tout.info(f'Found {len(mrs)} open pickman MR(s):') for merge_req in mrs: - tout.info(f" !{merge_req['iid']}: {merge_req['title']}") + tout.info(f" !{merge_req.iid}: {merge_req.title}") # Process any review comments on open MRs process_mr_reviews(remote, mrs) diff --git a/tools/pickman/ftest.py b/tools/pickman/ftest.py index 9c65652f27a..4459f108496 100644 --- a/tools/pickman/ftest.py +++ b/tools/pickman/ftest.py @@ -1233,11 +1233,13 @@ class TestStep(unittest.TestCase): def test_step_with_pending_mr(self): """Test step does nothing when MR is pending.""" - mock_mr = { - 'iid': 123, - 'title': '[pickman] Test MR', - 'web_url': 'https://gitlab.com/mr/123', - } + mock_mr = gitlab_api.PickmanMr( + iid=123, + title='[pickman] Test MR', + web_url='https://gitlab.com/mr/123', + source_branch='cherry-test', + description='Test', + ) with mock.patch.object(gitlab_api, 'get_merged_pickman_mrs', return_value=[]): with mock.patch.object(gitlab_api, 'get_open_pickman_mrs', @@ -1864,11 +1866,13 @@ class TestDoReviewWithMrs(unittest.TestCase): """Test review with open MRs but no comments.""" tout.init(tout.INFO) - mock_mr = { - 'iid': 123, - 'title': '[pickman] Test MR', - 'web_url': 'https://gitlab.com/mr/123', - } + mock_mr = gitlab_api.PickmanMr( + iid=123, + title='[pickman] Test MR', + web_url='https://gitlab.com/mr/123', + source_branch='cherry-test', + description='Test', + ) with mock.patch.object(gitlab_api, 'get_open_pickman_mrs', return_value=[mock_mr]): with mock.patch.object(gitlab_api, 'get_mr_comments', diff --git a/tools/pickman/gitlab_api.py b/tools/pickman/gitlab_api.py index 6720e0a1526..d0128e13f80 100644 --- a/tools/pickman/gitlab_api.py +++ b/tools/pickman/gitlab_api.py @@ -5,6 +5,7 @@ # """GitLab integration for pickman - push branches and create merge requests.""" +from collections import namedtuple import os import re import sys @@ -25,6 +26,17 @@ except ImportError: AVAILABLE = False +# Merge request info returned by get_pickman_mrs() +PickmanMr = namedtuple('PickmanMr', [ + 'iid', 'title', 'web_url', 'source_branch', 'description' +]) + +# Comment info returned by get_mr_comments() +MrComment = namedtuple('MrComment', [ + 'id', 'author', 'body', 'created_at', 'resolvable', 'resolved' +]) + + def check_available(): """Check if the python-gitlab module is available @@ -160,8 +172,7 @@ def get_pickman_mrs(remote, state='opened'): state (str): MR state ('opened', 'merged', 'closed', 'all') Returns: - list: List of dicts with 'iid', 'title', 'web_url', 'source_branch', - 'description' keys, or None on failure + list: List of PickmanMr tuples, or None on failure """ if not check_available(): return None @@ -186,13 +197,13 @@ def get_pickman_mrs(remote, state='opened'): pickman_mrs = [] for merge_req in mrs: if '[pickman]' in merge_req.title: - pickman_mrs.append({ - 'iid': merge_req.iid, - 'title': merge_req.title, - 'web_url': merge_req.web_url, - 'source_branch': merge_req.source_branch, - 'description': merge_req.description or '', - }) + pickman_mrs.append(PickmanMr( + iid=merge_req.iid, + title=merge_req.title, + web_url=merge_req.web_url, + source_branch=merge_req.source_branch, + description=merge_req.description or '', + )) return pickman_mrs except gitlab.exceptions.GitlabError as exc: tout.error(f'GitLab API error: {exc}') @@ -233,8 +244,7 @@ def get_mr_comments(remote, mr_iid): mr_iid (int): Merge request IID Returns: - list: List of dicts with 'id', 'author', 'body', 'created_at', - 'resolvable', 'resolved' keys, or None on failure + list: List of MrComment tuples, or None on failure """ if not check_available(): return None @@ -260,14 +270,14 @@ def get_mr_comments(remote, mr_iid): # Skip system notes (merge status, etc.) if note.system: continue - comments.append({ - 'id': note.id, - 'author': note.author['username'], - 'body': note.body, - 'created_at': note.created_at, - 'resolvable': getattr(note, 'resolvable', False), - 'resolved': getattr(note, 'resolved', False), - }) + comments.append(MrComment( + id=note.id, + author=note.author['username'], + body=note.body, + created_at=note.created_at, + resolvable=getattr(note, 'resolvable', False), + resolved=getattr(note, 'resolved', False), + )) return comments except gitlab.exceptions.GitlabError as exc: tout.error(f'GitLab API error: {exc}') -- 2.43.0