From 572b95e69863d65d9f9a329ff254c55a47768139 Mon Sep 17 00:00:00 2001 From: "qodo-code-review[bot]" <151058649+qodo-code-review[bot]@users.noreply.github.com> Date: Thu, 22 Jan 2026 19:13:48 +0000 Subject: [PATCH 1/7] Generated best practices file --- best_practices.md | 251 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 251 insertions(+) create mode 100644 best_practices.md diff --git a/best_practices.md b/best_practices.md new file mode 100644 index 00000000..d246068e --- /dev/null +++ b/best_practices.md @@ -0,0 +1,251 @@ + +Pattern 1: Add explicit unit tests for each early-exit/guard branch introduced in a PR (especially for None/empty config values), so behavior is specified and regressions are caught. + + +Example code before: +``` +def sync_fields(issue, updates): + if "github_project_fields" not in updates: + return + fields = issue.downstream.get("github_project_fields") + if fields is None: + return + update_fields(fields) +``` + +Example code after: +``` +def sync_fields(issue, updates): + fields = issue.downstream.get("github_project_fields") or {} + if "github_project_fields" not in updates or not fields: + return + update_fields(fields) + +def test_sync_fields_early_exit_no_updates(): + assert sync_fields(issue, updates=[]) is None + +def test_sync_fields_early_exit_none_fields(): + issue.downstream["github_project_fields"] = None + assert sync_fields(issue, updates=["github_project_fields"]) is None + +def test_sync_fields_happy_path_updates_fields(mocker): + issue.downstream["github_project_fields"] = {"priority": "P1"} + spy = mocker.spy(mod, "update_fields") + sync_fields(issue, updates=["github_project_fields"]) + spy.assert_called_once() +``` + +
Examples for relevant past discussions: + +- https://github.com/release-engineering/Sync2Jira/pull/405#discussion_r2683208326 +- https://github.com/release-engineering/Sync2Jira/pull/405#discussion_r2683211908 +- https://github.com/release-engineering/Sync2Jira/pull/404#discussion_r2665584806 +
+ + +___ + +Pattern 2: Do not silently continue after failures in critical integration paths (e.g., building Jira field caches, Snowflake connectivity, custom-field resolution); log and re-raise (or raise a clear ValueError) so the run fails loudly and predictably. + + +Example code before: +``` +def build_cache(client): + try: + cache.clear() + cache.update({f["name"]: f["id"] for f in client.fields()}) + except Exception as e: + log.error("Failed: %s", e) + return # continues with partial/bad state +``` + +Example code after: +``` +def build_cache(client): + cache.clear() + try: + for f in client.fields(): + cache[f["name"]] = f["id"] + except Exception as e: + log.error("Failed to build field cache: %s", e) + raise + +def resolve_field_id(client, name): + field_id = cache.get(name) + if not field_id: + raise ValueError(f"Could not resolve custom field '{name}' to an ID") + return field_id +``` + +
Examples for relevant past discussions: + +- https://github.com/release-engineering/Sync2Jira/pull/375#discussion_r2403003292 +- https://github.com/release-engineering/Sync2Jira/pull/375#discussion_r2403189630 +- https://github.com/release-engineering/Sync2Jira/pull/404#discussion_r2665525408 +- https://github.com/release-engineering/Sync2Jira/pull/399#discussion_r2662130914 +- https://github.com/release-engineering/Sync2Jira/pull/399#discussion_r2594056370 +
+ + +___ + +Pattern 3: Keep try/except blocks minimal and scoped to the single call that can reasonably fail; prefer context managers for resources, and avoid broad excepts that mask unrelated bugs. + + +Example code before: +``` +try: + conn = connect() + cur = conn.cursor() + cur.execute(sql) + rows = cur.fetchall() + cur.close() + conn.close() +except Exception: + return [] +``` + +Example code after: +``` +with connect() as conn: + cur = conn.cursor() + cur.execute(sql) + rows = cur.fetchall() + cur.close() +return rows +``` + +
Examples for relevant past discussions: + +- https://github.com/release-engineering/Sync2Jira/pull/404#discussion_r2665550385 +- https://github.com/release-engineering/Sync2Jira/pull/399#discussion_r2593928187 +- https://github.com/release-engineering/Sync2Jira/pull/375#discussion_r2403189630 +- https://github.com/release-engineering/Sync2Jira/pull/399#discussion_r2662130914 +
+ + +___ + +Pattern 4: Be intentional about logging volume: remove low-value INFO logs, downgrade noisy logs to DEBUG, and avoid redundant logging before raising exceptions to prevent massive log growth in production. + + +Example code before: +``` +log.info("About to sync PR %s", pr.url) +log.info("Syncing PR %s", pr.url) +try: + do_work() +except Exception as e: + log.error("Failed: %s", e) + raise +``` + +Example code after: +``` +log.debug("Syncing PR %s", pr.url) +do_work() # exceptions will be logged by the top-level handler with traceback +``` + +
Examples for relevant past discussions: + +- https://github.com/release-engineering/Sync2Jira/pull/405#discussion_r2683150750 +- https://github.com/release-engineering/Sync2Jira/pull/405#discussion_r2683072080 +- https://github.com/release-engineering/Sync2Jira/pull/399#discussion_r2593995729 +- https://github.com/release-engineering/Sync2Jira/pull/404#discussion_r2665525408 +
+ + +___ + +Pattern 5: Use Pythonic truthiness checks and short-circuiting to both simplify code and avoid unnecessary work (e.g., `if mapping:` instead of `len(mapping) > 0`, and check cheap conditions before expensive iterations). + + +Example code before: +``` +if "github_project_fields" in updates and len(github_project_fields) > 0: + update(github_project_fields) + +if any("transition" in item for item in issue_updates): + description = issue.status + "\n" + description +``` + +Example code after: +``` +if "github_project_fields" in updates and github_project_fields: + update(github_project_fields) + +if issue.status and any("transition" in item for item in issue_updates): + description = issue.status + "\n" + description +``` + +
Examples for relevant past discussions: + +- https://github.com/release-engineering/Sync2Jira/pull/405#discussion_r2683003809 +- https://github.com/release-engineering/Sync2Jira/pull/405#discussion_r2683011856 +- https://github.com/release-engineering/Sync2Jira/pull/298#discussion_r1975579347 +
+ + +___ + +Pattern 6: Cache stable identifiers (e.g., Jira issue key/ID) rather than ORM/resource objects, and refresh/fetch the current object when needed to avoid stale data. + + +Example code before: +``` +cache[url] = jira_issue_obj # mutable/stale ORM object + +def get_issue(url): + return cache.get(url) +``` + +Example code after: +``` +cache[url] = jira_issue_key # stable identifier + +def get_issue(client, url): + key = cache.get(url) + return client.issue(key) if key else None +``` + +
Examples for relevant past discussions: + +- https://github.com/release-engineering/Sync2Jira/pull/398#discussion_r2569771392 +
+ + +___ + +Pattern 7: Keep tests focused and maintainable: assert the right behavior (not incidental implementation details), mock helper functions instead of re-testing internals, prefer consistent patching patterns (e.g., decorators/patch.dict), and improve readability by using local variables when formatting tools create noise. + + +Example code before: +``` +@mock.patch("module.issue_handlers") # dict patched as attribute +def test_x(self, handlers): + handlers["k"].return_value = None + +self.assertEqual(len(cache), 4) # incidental, not spec +``` + +Example code after: +``` +@mock.patch.dict("module.issue_handlers", {"k": MagicMock(return_value=None)}) +def test_x(self): + ... + +# Assert explicit contents / invariant instead of incidental size +self.assertEqual(cache, {"priority": "priority", "summary": "summary"}) +``` + +
Examples for relevant past discussions: + +- https://github.com/release-engineering/Sync2Jira/pull/404#discussion_r2665572801 +- https://github.com/release-engineering/Sync2Jira/pull/399#discussion_r2594045631 +- https://github.com/release-engineering/Sync2Jira/pull/375#discussion_r2403221096 +- https://github.com/release-engineering/Sync2Jira/pull/289#discussion_r1941816872 +- https://github.com/release-engineering/Sync2Jira/pull/404#discussion_r2669219878 +
+ + +___ From 6257efc60191ef633185729f5d792b80b9c1b802 Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Tue, 27 Jan 2026 08:04:37 -0500 Subject: [PATCH 2/7] Reframe Pattern 1 to emphasize 100% test coverage aspiration Replace the specific focus on early-exit/guard branches with a broader principle: unit tests should aspire to 100% coverage of all normal execution paths and reasonably testable error cases. This reframing provides clearer guidance that PRs should include tests for new code paths and should not reduce overall coverage. Assisted-by: Claude Sonnet 4.5 Signed-off-by: Ralph Bean --- best_practices.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/best_practices.md b/best_practices.md index d246068e..ea905e2b 100644 --- a/best_practices.md +++ b/best_practices.md @@ -1,5 +1,7 @@ -Pattern 1: Add explicit unit tests for each early-exit/guard branch introduced in a PR (especially for None/empty config values), so behavior is specified and regressions are caught. +Pattern 1: Unit tests should aspire to provide 100% coverage of the code. All normal-execution paths, including early-exit/guard conditionals, should be exercised. While it is not strictly necessary to exercise every error case, all cases which can be reasonably tested should be. + +Changes to the code which create new functions or branches should include unit tests which provide coverage for those paths. As a general rule, it should not be the case that a PR causes the overall coverage to be reduced (although, changes which add no uncovered code but which delete more code than they add can have this effect, but these are unusual). Example code before: From 616ccec524ebff136f7e207f640327891edc9163 Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Tue, 27 Jan 2026 08:04:54 -0500 Subject: [PATCH 3/7] Expand Pattern 2 with three-part emphasis on exception handling Add detailed guidance explaining the three key aspects of handling critical failures: don't continue after failures, raise exceptions to allow event-level recovery without crashing the entire tool, and ensure sufficient logging context for debugging. This clarifies the balance between resilience and fail-fast behavior. Assisted-by: Claude Sonnet 4.5 Signed-off-by: Ralph Bean --- best_practices.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/best_practices.md b/best_practices.md index ea905e2b..79a23714 100644 --- a/best_practices.md +++ b/best_practices.md @@ -48,6 +48,11 @@ def test_sync_fields_happy_path_updates_fields(mocker): ___ Pattern 2: Do not silently continue after failures in critical integration paths (e.g., building Jira field caches, Snowflake connectivity, custom-field resolution); log and re-raise (or raise a clear ValueError) so the run fails loudly and predictably. + +There are three parts to emphasize: +- Do not attempt to continue after a critical failure +- Instead, raise an exception which will propagate back to the base of the stack which will cause the tool to discard the current event and continue with the next one: we want the tool to recover and continue running, if possible and reasonable, but we don't want to perform any heroic measures on the part of any single event. (Having the tool hard-crash is generally unuseful, because the infrastructure will restart it, the restart will open a new log, and the original context will be lost.) +- Log information required to understand and address the failure, either at the point of failure or via the raised exception, which will be logged when the stack is unwound. Example code before: From a8915a62352cb46db39e73737a896f82442789ba Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Tue, 27 Jan 2026 08:05:12 -0500 Subject: [PATCH 4/7] Expand Pattern 3 with detailed guidance on focused try blocks Add comprehensive explanation of three key principles for exception handling: focus try blocks on single failure points with clear except actions, use specific exception targets with hierarchical exception classes where appropriate, and consider context managers as alternatives. This provides clearer guidance on writing robust exception handling code. Assisted-by: Claude Sonnet 4.5 Signed-off-by: Ralph Bean --- best_practices.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/best_practices.md b/best_practices.md index 79a23714..b974b4da 100644 --- a/best_practices.md +++ b/best_practices.md @@ -97,6 +97,12 @@ def resolve_field_id(client, name): ___ Pattern 3: Keep try/except blocks minimal and scoped to the single call that can reasonably fail; prefer context managers for resources, and avoid broad excepts that mask unrelated bugs. + +The implementation of `try` blocks should be focused on the actions of the `except` clauses: there should be only "one thing" which can go wrong in the body of the `try` block, so that which exceptions are to be caught and what their respective actions should be is clear. Statements which are related to the contents of the `try` block should be placed before or after the block and not included in it, to avoid the possibility of them raising unexpected errors which might trigger erroneous handling. + +Similarly, the targets of `except` clauses should be as specific and explicit as reasonably possible. Using hierarchical exception classes is an excellent approach to simplifying exception handling which allows a single clause to handle any of a set of related exceptions. + +Context managers can be a superb alternative to `try` blocks. Example code before: From b9e9115ab7fa8b0aa987ca230ccd86781a56d480 Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Tue, 27 Jan 2026 08:05:34 -0500 Subject: [PATCH 5/7] Remove Pattern 6 about caching stable identifiers Remove the pattern about caching stable identifiers versus ORM objects as it is too specific in scope for a general best practices list. The remaining patterns have been renumbered accordingly (Pattern 7 becomes Pattern 6). Assisted-by: Claude Sonnet 4.5 Signed-off-by: Ralph Bean --- best_practices.md | 30 +----------------------------- 1 file changed, 1 insertion(+), 29 deletions(-) diff --git a/best_practices.md b/best_practices.md index b974b4da..fdc266a9 100644 --- a/best_practices.md +++ b/best_practices.md @@ -201,35 +201,7 @@ if issue.status and any("transition" in item for item in issue_updates): ___ -Pattern 6: Cache stable identifiers (e.g., Jira issue key/ID) rather than ORM/resource objects, and refresh/fetch the current object when needed to avoid stale data. - - -Example code before: -``` -cache[url] = jira_issue_obj # mutable/stale ORM object - -def get_issue(url): - return cache.get(url) -``` - -Example code after: -``` -cache[url] = jira_issue_key # stable identifier - -def get_issue(client, url): - key = cache.get(url) - return client.issue(key) if key else None -``` - -
Examples for relevant past discussions: - -- https://github.com/release-engineering/Sync2Jira/pull/398#discussion_r2569771392 -
- - -___ - -Pattern 7: Keep tests focused and maintainable: assert the right behavior (not incidental implementation details), mock helper functions instead of re-testing internals, prefer consistent patching patterns (e.g., decorators/patch.dict), and improve readability by using local variables when formatting tools create noise. +Pattern 6: Keep tests focused and maintainable: assert the right behavior (not incidental implementation details), mock helper functions instead of re-testing internals, prefer consistent patching patterns (e.g., decorators/patch.dict), and improve readability by using local variables when formatting tools create noise. Example code before: From 0b368b09bd90393f2a12f886b4afe98fe8375dbf Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Tue, 27 Jan 2026 08:05:57 -0500 Subject: [PATCH 6/7] Expand Pattern 6 with four-part breakdown on test maintainability Add detailed explanation breaking down the grab-bag pattern into four distinct principles: testing behaviors versus implementation details, understanding unit scope for efficient mocking, maintaining consistent patching patterns across tests, and using readability tricks when Black's formatting creates noise. This clarifies the multiple concerns addressed by this pattern. Assisted-by: Claude Sonnet 4.5 Signed-off-by: Ralph Bean --- best_practices.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/best_practices.md b/best_practices.md index fdc266a9..28b1d65a 100644 --- a/best_practices.md +++ b/best_practices.md @@ -202,6 +202,12 @@ if issue.status and any("transition" in item for item in issue_updates): ___ Pattern 6: Keep tests focused and maintainable: assert the right behavior (not incidental implementation details), mock helper functions instead of re-testing internals, prefer consistent patching patterns (e.g., decorators/patch.dict), and improve readability by using local variables when formatting tools create noise. + +This pattern addresses several aspects of writing maintainable tests: +- Unit test assertions should test intended (visible) behaviors and not implementation details. The external behavior is what we care about, and we want to leave the internals free to be refactored or rearchitected. +- We should be conscious of what constitutes the "unit" that we are testing. In most cases, it is simpler to test a single function with its subroutines replaced by mocks. However, this is not always the case, and so the approach should generally reflect whatever is most expedient. +- Be consistent about how we patch things, so that readers and subsequent developers are not surprised and don't have to embrace a new idiom for each test they encounter. +- When Black makes the code less readable, feel free to use tricks like introducing local variables or using single strings (which Black won't break) or putting breaks in strings (which Python will automatically re-join) to restore readability. Example code before: From 1e79eb4b9520983d37f1eb09754daca7358d480e Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Tue, 27 Jan 2026 14:24:29 -0500 Subject: [PATCH 7/7] Split Black readability guidance into separate pattern Move the guidance about working around Black's formatting from Pattern 6 (testing) into a new Pattern 7 focused specifically on code readability. This makes it clearer that the Black guidance applies to all code, not just tests. Assisted-by: Claude Sonnet 4.5 Signed-off-by: Ralph Bean --- best_practices.md | 32 ++++++++++++++++++++++++++++++-- 1 file changed, 30 insertions(+), 2 deletions(-) diff --git a/best_practices.md b/best_practices.md index 28b1d65a..339d39c4 100644 --- a/best_practices.md +++ b/best_practices.md @@ -201,13 +201,12 @@ if issue.status and any("transition" in item for item in issue_updates): ___ -Pattern 6: Keep tests focused and maintainable: assert the right behavior (not incidental implementation details), mock helper functions instead of re-testing internals, prefer consistent patching patterns (e.g., decorators/patch.dict), and improve readability by using local variables when formatting tools create noise. +Pattern 6: Keep tests focused and maintainable: assert the right behavior (not incidental implementation details), mock helper functions instead of re-testing internals, and prefer consistent patching patterns (e.g., decorators/patch.dict). This pattern addresses several aspects of writing maintainable tests: - Unit test assertions should test intended (visible) behaviors and not implementation details. The external behavior is what we care about, and we want to leave the internals free to be refactored or rearchitected. - We should be conscious of what constitutes the "unit" that we are testing. In most cases, it is simpler to test a single function with its subroutines replaced by mocks. However, this is not always the case, and so the approach should generally reflect whatever is most expedient. - Be consistent about how we patch things, so that readers and subsequent developers are not surprised and don't have to embrace a new idiom for each test they encounter. -- When Black makes the code less readable, feel free to use tricks like introducing local variables or using single strings (which Black won't break) or putting breaks in strings (which Python will automatically re-join) to restore readability. Example code before: @@ -239,4 +238,33 @@ self.assertEqual(cache, {"priority": "priority", "summary": "summary"}) +___ + +Pattern 7: When Black makes the code less readable, feel free to use tricks like introducing local variables, using single strings (which Black won't break), or putting breaks in strings (which Python will automatically re-join) to restore readability. + + +Example code before: +``` +# Black reformats this to be less readable +log.error( + "Failed to sync %s/%s from %s to JIRA project %s component %s", + repo_owner, + repo_name, + upstream_url, + jira_project_key, + jira_component, +) +``` + +Example code after: +``` +# Use a local variable to keep it as a single line +msg = "Failed to sync %s/%s from %s to JIRA project %s component %s" +log.error(msg, repo_owner, repo_name, upstream_url, jira_project_key, jira_component) + +# Or use a single string that Black won't break +log.error("Failed to sync %s/%s from %s to JIRA project %s component %s", repo_owner, repo_name, upstream_url, jira_project_key, jira_component) +``` + + ___