[Concept,10/24] pickman: Use named tuples for MR data

Message ID 20251217022823.392557-11-sjg@u-boot.org
State New
Headers
Series pickman: Refine the feature set |

Commit Message

Simon Glass Dec. 17, 2025, 2:27 a.m. UTC
  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(-)
  

Patch

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}')