Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Dec 24, 2025

Applied code quality and correctness fixes addressing inconsistent dictionary access, fragile timestamp parsing, and unclear constant naming.

Changes

  • Fixed dictionary iteration bug in get_slack_markdown_by_linear_username: Changed from iterating over config["people"] keys while accessing as values, to correctly iterating over .values() with direct attribute access via .get()

  • Replaced manual timestamp string manipulation with explicit parsing: Used datetime.strptime() with format string "%Y-%m-%dT%H:%M:%SZ" and explicit UTC timezone instead of stripping 'Z' suffix and calling fromisoformat(). Applied timezone-aware comparison using datetime.now(timezone.utc)

  • Renamed RETRY_COUNT to MAX_RETRY_COUNT: Clearer naming following conventions, updated all references including docstring

# Before
for person in config["people"]:
    if config["people"][person]["linear_username"] == username:
        return f"<@{config['people'][person]['slack_id']}>"

# After  
for person in config["people"].values():
    if person.get("linear_username") == username:
        return f"<@{person['slack_id']}>"
Original prompt
Please apply the following diffs and create a pull request.
Once the PR is ready, give it a title based on the messages of the fixes being applied.

[{"message":"This code iterates over the keys of config['people'] dictionary but then accesses config['people'][person], which is inconsistent with line 147 that correctly uses .values(). This should be 'for person in config[\"people\"].values():' and then access person['linear_username'] directly.","fixFiles":[{"filePath":"jobs.py","diff":"diff --git a/jobs.py b/jobs.py\n--- a/jobs.py\n+++ b/jobs.py\n@@ -129,9 +129,9 @@\n         return \"No Assignee\"\n \n     config = load_config()\n-    for person in config[\"people\"]:\n-        if config[\"people\"][person][\"linear_username\"] == username:\n-            return f\"<@{config['people'][person]['slack_id']}>\"\n+    for person in config[\"people\"].values():\n+        if person.get(\"linear_username\") == username:\n+            return f\"<@{person['slack_id']}>\"\n     return \"No Assignee\"\n \n \n"}]},{"message":"This line accesses config['people'][person]['slack_id'] but 'person' is a key from the iteration on line 132, making this equivalent to config['people'][key]['slack_id']. Should be person['slack_id'] if the iteration is fixed to use .values().","fixFiles":[{"filePath":"jobs.py","diff":"diff --git a/jobs.py b/jobs.py\n--- a/jobs.py\n+++ b/jobs.py\n@@ -129,9 +129,9 @@\n         return \"No Assignee\"\n \n     config = load_config()\n-    for person in config[\"people\"]:\n-        if config[\"people\"][person][\"linear_username\"] == username:\n-            return f\"<@{config['people'][person]['slack_id']}>\"\n+    for person in config[\"people\"].values():\n+        if person.get(\"linear_username\") == username:\n+            return f\"<@{person['slack_id']}>\"\n     return \"No Assignee\"\n \n \n"}]},{"message":"The manual handling of ISO 8601 timestamps by removing 'Z' suffix is error-prone. Consider using datetime.fromisoformat() with proper timezone handling or use dateutil.parser for more robust timestamp parsing.","fixFiles":[{"filePath":"jobs.py","diff":"diff --git a/jobs.py b/jobs.py\n--- a/jobs.py\n+++ b/jobs.py\n@@ -454,10 +454,11 @@\n                 ]\n                 if events:\n                     created = max(ev[\"createdAt\"] for ev in events)\n-                    if created.endswith(\"Z\"):\n-                        created = created[:-1]\n-                    dt = datetime.fromisoformat(created)\n-                    days_waiting = (datetime.now() - dt).days\n+                    # Parse GitHub-style ISO 8601 timestamp with explicit UTC timezone\n+                    dt = datetime.strptime(created, \"%Y-%m-%dT%H:%M:%SZ\").replace(\n+                        tzinfo=timezone.utc\n+                    )\n+                    days_waiting = (datetime.now(timezone.utc) - dt).days\n                 else:\n                     days_waiting = 0\n                 pr_days.append((days_waiting, pr))\n"}]},{"message":"Using datetime.now() without timezone information to compare with a parsed datetime that may have timezone info can lead to incorrect calculations. Use datetime.now(timezone.utc) for consistency with other datetime operations in the file.","fixFiles":[{"filePath":"jobs.py","diff":"diff --git a/jobs.py b/jobs.py\n--- a/jobs.py\n+++ b/jobs.py\n@@ -456,8 +456,8 @@\n                     created = max(ev[\"createdAt\"] for ev in events)\n                     if created.endswith(\"Z\"):\n                         created = created[:-1]\n-                    dt = datetime.fromisoformat(created)\n-                    days_waiting = (datetime.now() - dt).days\n+                    dt = datetime.fromisoformat(created).replace(tzinfo=timezone.utc)\n+                    days_waiting = (datetime.now(timezone.utc) - dt).days\n                 else:\n                     days_waiting = 0\n                 pr_days.append((days_waiting, pr))\n"}]},{"message":"[nitpick] Constants should follow UPPER_SNAKE_CASE naming convention consistently. While this is already correct, consider using MAX_RETRY_COUNT for clarity about what the number represents.","fixFiles":[{"filePath":"jobs.py","diff":"diff --git a/jobs.py b/jobs.py\n--- a/jobs.py\n+++ b/jobs.py\n@@ -35,7 +35,7 @@\n load_dotenv()\n \n # Retry configuration for the with_retries decorator.\n-RETRY_COUNT = 3\n+MAX_RETRY_COUNT = 3\n RETRY_SLEEP_SECONDS = 5\n \n \n@@ -95,7 +95,7 @@\n     \"\"\"\n     @wraps(func)\n     def wrapper(*args, **kwargs):\n-        for attempt in range(RETRY_COUNT):\n+        for attempt in range(MAX_RETRY_COUNT):\n             try:\n                 return func(*args, **kwargs)\n             except Exception as e:\n@@ -105,7 +105,7 @@\n                     type(e).__name__,\n                     exc_info=True,\n                 )\n-                if attempt == RETRY_COUNT - 1:\n+                if attempt == MAX_RETRY_COUNT - 1:\n                     raise\n                 time.sleep(RETRY_SLEEP_S...

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

…onstant

Co-authored-by: redreceipt <2659478+redreceipt@users.noreply.github.com>
@redreceipt redreceipt temporarily deployed to bug-board-copilot-fix-i-x4mt7e December 24, 2025 16:02 Inactive
Co-authored-by: redreceipt <2659478+redreceipt@users.noreply.github.com>
@redreceipt redreceipt temporarily deployed to bug-board-copilot-fix-i-x4mt7e December 24, 2025 16:03 Inactive
Copilot AI changed the title [WIP] Fix inconsistent dictionary access in jobs.py Fix dictionary iteration, timestamp parsing, and constant naming in jobs.py Dec 24, 2025
Copilot AI requested a review from redreceipt December 24, 2025 16:04
@redreceipt redreceipt marked this pull request as ready for review December 24, 2025 16:06
@redreceipt redreceipt merged commit a981351 into main Dec 24, 2025
7 checks passed
@redreceipt redreceipt deleted the copilot/fix-inconsistent-access-people branch December 24, 2025 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants