Repository navigation
Round 3 follow-up: hoist to_string()'s loop body out in activity/models.py - #560
Merged
Merged
Conversation
…ls.py My earlier split in #551 (merged) extracted the per-argument-type formatting into _format_*_arg() helpers, but the 8 isinstance-based if-checks in to_string()'s main loop stayed inline, nested inside a for loop. Sonar's cognitive complexity model multiplies nested-structure penalties, so to_string() was re-flagged at complexity 30 (even though each individual helper is now simple) once re-analyzed on stage-eks after merge. Extracted the entire loop body into _apply_arg_to_accumulators(arg, arguments, acc), called once per arg from to_string()'s loop. Moving it out of the for loop removes Sonar's nesting multiplier for each of the independent if-checks, since the method itself has no loop around them. Body moved verbatim, including: - the asymmetric "not X" guards (only the first matching arg of a given type is kept; the Tag branch is the only one that does not remove its arg from `arguments`, which was already true before this change); - the CHANGE_STATUS branch's condition, which can independently match the same arg that already matched an isinstance branch above it (e.g. a File arg during a CHANGE_STATUS action) and call arguments.remove(arg) a second time on it. This existing behavior (including its risk of raising ValueError at runtime) is preserved exactly rather than silently fixed, since that's out of scope for a complexity-only refactor. addon/rating/version/collection/tag/group/file_/status move from local variables into a single `acc` dict threaded through the helper and back into to_string()'s kw dict construction; no value or key changes.
Sonar's PR scan flagged the newly-extracted _apply_arg_to_accumulators() itself at complexity 16/15 - hoisting the loop body out of to_string() wasn't enough on its own, since 8 independent if-checks in one non-nested method was still over the threshold. Split it into three sub-helpers, keeping branch order and the arguments.remove() calls exactly where they were: - _apply_arg_isinstance_checks_1(): Addon/Rating/Version/Collection (4) - _apply_arg_isinstance_checks_2(): Tag/Group/File (3) - _apply_arg_change_status(): the CHANGE_STATUS branch (1) _apply_arg_to_accumulators() now just calls the three in sequence. Updated its docstring to describe the two-stage extraction accurately.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of #450 (round 3).
Sonar issue: python:S3776,
activity/models.py:556, complexity 30.My earlier split in thunderbird/addons-server#551 (merged) extracted the per-argument-type formatting into
_format_*_arg()helpers, but the 8isinstance-based if-checks into_string()'s main loop stayed inline, nested inside aforloop. Sonar's cognitive complexity model multiplies nested-structure penalties, soto_string()was re-flagged at complexity 30 (even though each individual helper is now simple) once re-analyzed onstage-eksafter merge.Extracted the entire loop body into
_apply_arg_to_accumulators(arg, arguments, acc), called once per arg fromto_string()'s loop. Moving it out of theforloop removes Sonar's nesting multiplier for each of the independent if-checks, since the method itself has no loop around them.Body moved verbatim, including some subtle existing behavior I deliberately did not "fix" as part of this complexity-only change:
not Xguards (only the first matching arg of a given type is kept; theTagbranch is the only one that does not remove its arg fromarguments, which was already true before this change);CHANGE_STATUSbranch's condition, which can independently match the same arg that already matched anisinstancebranch above it (e.g. aFilearg during aCHANGE_STATUSaction) and callarguments.remove(arg)a second time on it. This existing behavior (including its risk of raisingValueErrorat runtime in that edge case) is preserved exactly.addon/rating/version/collection/tag/group/file_/statusmove from local variables into a singleaccdict threaded through the helper and back intoto_string()'skwdict construction; no value or key changes.Local Greptile review: org free-tier quota exhausted, skipped per coordinator guidance; the Greptile GitHub check on this PR still runs independently.