From patchwork Wed Dec 17 02:27:59 2025 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Simon Glass X-Patchwork-Id: 953 Return-Path: X-Original-To: u-boot-concept@u-boot.org Delivered-To: u-boot-concept@u-boot.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=u-boot.org; s=default; t=1765938571; bh=ySek2ehoEN3jKTFKNaoKfOEy2h1iRbel4hnae5F0doM=; h=From:To:Date:In-Reply-To:References:CC:Subject:List-Id: List-Archive:List-Help:List-Owner:List-Post:List-Subscribe: List-Unsubscribe:From; b=UWF3f3KWJBcML1W75OS4YB83GsG8LHJaPDB1bCbEhEyYG644AOsH//ejwHbMIorz6 5ZsWWy2lV+ti/kj1W4BeMuFsbwASwfzJVTtleYHOu+MoNGVLFiNhGtInZD2a5mp/rt eMeSwxBPdXdhyWeVZ3JMF8+HaMOuze0qIJNwRgMfVbUIsGeJmj3VnuCx6xdEc/b58V f5FCg7Jxj8iouIETqOQaMyM381efNSd1/l30AU/wIFLdNesluM5DptbxwB+ozlnQ82 L0McLYnD5dh3Iv+jx6A3h4sZD6jCTaeuI+luJ9ua9nhHtqUNY82heybWD0Hj1nrydw QevhwPxR3lWkg== Received: from localhost (localhost [127.0.0.1]) by mail.u-boot.org (Postfix) with ESMTP id BCE8B68BC1 for ; Tue, 16 Dec 2025 19:29:31 -0700 (MST) X-Virus-Scanned: Debian amavis at Received: from mail.u-boot.org ([127.0.0.1]) by localhost (mail.u-boot.org [127.0.0.1]) (amavis, port 10024) with ESMTP id a8qij1WUMHuL for ; Tue, 16 Dec 2025 19:29:31 -0700 (MST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=u-boot.org; s=default; t=1765938571; bh=ySek2ehoEN3jKTFKNaoKfOEy2h1iRbel4hnae5F0doM=; h=From:To:Date:In-Reply-To:References:CC:Subject:List-Id: List-Archive:List-Help:List-Owner:List-Post:List-Subscribe: List-Unsubscribe:From; b=UWF3f3KWJBcML1W75OS4YB83GsG8LHJaPDB1bCbEhEyYG644AOsH//ejwHbMIorz6 5ZsWWy2lV+ti/kj1W4BeMuFsbwASwfzJVTtleYHOu+MoNGVLFiNhGtInZD2a5mp/rt eMeSwxBPdXdhyWeVZ3JMF8+HaMOuze0qIJNwRgMfVbUIsGeJmj3VnuCx6xdEc/b58V f5FCg7Jxj8iouIETqOQaMyM381efNSd1/l30AU/wIFLdNesluM5DptbxwB+ozlnQ82 L0McLYnD5dh3Iv+jx6A3h4sZD6jCTaeuI+luJ9ua9nhHtqUNY82heybWD0Hj1nrydw QevhwPxR3lWkg== Received: from mail.u-boot.org (localhost [127.0.0.1]) by mail.u-boot.org (Postfix) with ESMTP id A9AED68AFD for ; Tue, 16 Dec 2025 19:29:31 -0700 (MST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=u-boot.org; s=default; t=1765938569; bh=y95tHadaT6I3sL8pyzAIt+WIx6WkeJRfViWLYA6Ukss=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=sklOF9i+ui8dS7+4lRyEmWJt1wuM894IAQcXMu1JLBRLjlZAXmJZXRO2YsmrX2PQq qCGV6BUQO61z9/2LyHssQ8lk2Yer5km5+VOBasgxJhRyaLJ8Q3R/s6EzKlpaIyk70I IJi6rA5+NspNh7oWQRcOQaOfL/eKdL85S3fOchmhaKfKWyLM3Z+h2lSYdToE/Oo6WB RzvAw6mpHL3QU2ZNVGrDKNFnvzxJQc+4w2jV04JrJSwdBf88JYNehrI7Raww8HgSSn imeoNKsWSSM0gURC8LZN6bGsY5eulcxdIijd7bZKzTOJHRIkv5EuVRnCJ9aqq98z+V udrTBRapKWqdw== Received: from localhost (localhost [127.0.0.1]) by mail.u-boot.org (Postfix) with ESMTP id AE7A76884F; Tue, 16 Dec 2025 19:29:29 -0700 (MST) X-Virus-Scanned: Debian amavis at Received: from mail.u-boot.org ([127.0.0.1]) by localhost (mail.u-boot.org [127.0.0.1]) (amavis, port 10026) with ESMTP id iz3Bh64yQQb8; Tue, 16 Dec 2025 19:29:29 -0700 (MST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=u-boot.org; s=default; t=1765938565; bh=pmv33VMeeNSvm9jhFZY/nwOFxWEjKslRYM4qxZ7pdN8=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=U1zhJ2dZZUi/pKU+tpgNRph4+xvcbBe3MmwxvQbB9/PyqRFJFhudzQO7FjbgItwmg 3D5n4HTlkComcWD35hyKkMuQTAyXasZ64/w4L965pHshBeQ/xuGyb5LM1UCDr+XAYI 34XHkNaP9Cw9wx6VkNkipjBcpOB5TuZcxLTy5YAzyP9vU7l9itfvTCcdOLJQkTRfio s0fHnISNuyosqeiTtsM1AUycrVlKO8WC+wHiHJsR2Eo0D7Gag088xdValowrj8oMIV uAuRQSgdMpXqJ6mBDqttXMUQoXkWbFPfpuJxOh/KBKQY7TuV/bJ1o5J1g79xVrtEuj WKBCQp4V4mq4g== Received: from u-boot.org (unknown [73.34.74.121]) by mail.u-boot.org (Postfix) with ESMTPSA id 19FB468AFD; Tue, 16 Dec 2025 19:29:25 -0700 (MST) From: Simon Glass To: U-Boot Concept Date: Tue, 16 Dec 2025 19:27:59 -0700 Message-ID: <20251217022823.392557-11-sjg@u-boot.org> X-Mailer: git-send-email 2.43.0 In-Reply-To: <20251217022823.392557-1-sjg@u-boot.org> References: <20251217022823.392557-1-sjg@u-boot.org> MIME-Version: 1.0 Message-ID-Hash: 6BS2WT4UTSQVBIQYDYHAQP6NAE3NCEP3 X-Message-ID-Hash: 6BS2WT4UTSQVBIQYDYHAQP6NAE3NCEP3 X-MailFrom: sjg@u-boot.org X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: Simon Glass , "Claude Opus 4 . 5" X-Mailman-Version: 3.3.10 Precedence: list Subject: [Concept] [PATCH 10/24] pickman: Use named tuples for MR data List-Id: Discussion and patches related to U-Boot Concept Archived-At: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: From: Simon Glass 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 Signed-off-by: Simon Glass --- 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}')