refactor to reduce complexity by okurz · Pull Request #429 · openSUSE/qem-bot

okurz · 2026-03-09T17:59:27Z

refactor(types/submission): reduce cyclomatic complexity in init
refactor(gitea): reduce cyclomatic complexity of build processing
refactor(submissions): reduce cyclomatic complexity in Submissions
refactor(submission): reduce cyclomatic complexity in Submission
fix: clean up dead code identified by vulture

This is preparatory refactoring and cleanup work for poo#197357 to
reduce complexity preventing exceeding accepted threshold to be
ready for further rework.

Related progress issue: https://progress.opensuse.org/issues/197357

codecov · 2026-03-09T18:38:14Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (6c950e3) to head (5fb9462).
⚠️ Report is 6 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff            @@
##            master      #429   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           91        91           
  Lines         8571      8614   +43     
  Branches       457       460    +3     
=========================================
+ Hits          8571      8614   +43

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

asmorodskyi

see comments inline

asmorodskyi · 2026-03-10T08:34:04Z

-    if arch == "local":
-        return channel
-
+def _get_product_version(res: Any, project: str, product_name: str) -> str:  # noqa: ANN401


can you elaborate how splitting one function into two can be called "reduce complexity" ? it is hard to read such code . you need to jump from one place into another and with each jump you loosing ability to see full picture

That's not how one should read code. complexity means that the logic is spread over multiple levels. When reading the code where get_product_version is used you should just know about the promises that the function does, i.e. "take a project and product name and give me the product version as string". Then you follow the logic of the calling function. And independant of that you can check the implementation of "get_product_version" if you are interested. By the way in my commits I explicitly refer to "cyclomatic complexity" which we have metrics for and also acceptance thresholds so I am reducing the levels to have more headroom.

get_product_version is used you should just know about the promises

ok give me clear definition what product version is ? What possible formats it could have ?

I noted down a todo locally to look into this and potentially improve that. Alternatively we can record a github issue or a ticket. With this, are you fine to accept the state as-is?

@asmorodskyi I created #433 to show how a hardened type for "ProductVersion" would look like

asmorodskyi · 2026-03-10T08:50:06Z

-        self.rev_logged: bool = False
-        self.livepatch: bool = self.is_livepatch(self.packages)
+    @staticmethod
+    def _parse_channels(raw_channels: list[str]) -> tuple[list[Repos], set[str]]:


why do we have so many use of @staticmethod ? this is not how Object Oriented Programming suppose to work . class suppose to operate data in the class . static methods should be real exception not common approach

in this particular case you can avoid creation of _initialize_channels method but instead just put results of work of this method into self.channels, self.skipped_products

it depends. you have to lookup the sequence of the flow. My point is the comment is not very helpful without look in the big picture and the current design. And again, IMO another design conflict late in the PR rather focusing in the objective and the quality review (this comment is not a personal one about you @asmorodskyi - just a general observation that keeps appearing)

@asmorodskyi I kept my changes consistent with the existing design. I refactored many (maybe all?) "staticmethods" and please take a look into my draft change so that we can judge if we prefer such approach in general. Based on that we can decide if we want to forbid the use of the annotator "@staticmethod" completely:

RFC: refactor: move static methods to free functions #432

this is not how Object Oriented Programming suppose to work

Luckily nobody says we have to use OOP all the time.

Motivation: There was unused code in the project such as variables and empty classes. Additionally, some test names were missing an underscore, rendering them invisible to pytest and effectively making them dead code. Design Choices: - Fixed test function names by adding the missing '_' prefix after 'test' - Removed unused `default_flavor_suffix` from `incrementapprover.py` - Removed unused `no_retry` from `utils.py` - Removed unused `EmptySettingsError` exception class from `errors.py` User Benefits: - Improves maintainability by removing code that serves no purpose - Increases test coverage since previously ignored tests are now executed

Motivation: Two methods were identified by radon as having high cyclomatic complexity (Grade D and C). Complex methods are harder to read, maintain, and test. Design Choices: - Extracted channel parsing - Extracted architecture/version grouping - Used mapping and list comprehensions to simplify the grouping logic - Replaced deep if/else blocks with flat conditional statements User Benefits: - Improved long-term maintainability of the project - Easier to trace channel parsing logic when adding new channel formats

Motivation: The `Submissions.should_skip` and `Submissions.handle_submission` methods had high cyclomatic complexity (Grade C) which degrades maintainability and readability. Design Choices: - Extracted nested conditions in `should_skip` into private helper methods `_is_invalid_status` and `_is_invalid_package_or_channel`. - Extracted setting preparation logic from `handle_submission` into a new method `_prepare_settings` to simplify the main flow. - Used early returns instead of deeply nested if-statements to make the logic more linear. User Benefits: - Code is easier to digest and maintain - Achieved Grade A and B maintainability for the refactored methods

Motivation: The `add_build_result` and `add_channel_for_build_result` functions had high cyclomatic complexity (Grade C) which degrades maintainability. Design Choices: - Extracted product version retrieval into a new helper `_get_product_version` - Extracted scminfo logic into a new helper `_update_scminfo` - Extracted build status updating into a new helper `_update_build_statuses` - Kept the main flow of `add_build_result` clean and readable User Benefits: - Easier to test individual pieces of logic - Grade A maintainability achieved for all refactored blocks

Refactor __init__ into helper methods to reduce CC from 27 to manageable levels.

Design Choices: Replaced the `Any` typing for `res` parameters with `etree._Element` across `openqabot/loader/gitea.py` for methods interacting with lxml-parsed build results. Benefits: Better type safety, autocomplete, and maintainability.

asmorodskyi

ok'ish ...

mergify · 2026-03-10T13:21:30Z

This pull request, with head sha 5fb94620bf7378c0863d78052f6fe7621d6ee724, has been successfully merged with fast-forward by Mergify.

This pull request will be automatically closed by GitHub.

As soon as GitHub detects that the sha 5fb94620bf7378c0863d78052f6fe7621d6ee724 is part of the master branch, it will mark this pull request as merged.

It is possible for this pull request to remain open if this detection does not happen, this usually happens when a force-push is done on this branch feature/033_improve_code_quality, this means GitHub will fail to detect the merge.

Motivation: Product version extraction used plain strings and empty strings for missing values, causing ambiguity and potential errors. PR openSUSE#429 feedback requested stricter definitions and better type safety. Design Choices: - Introduced ProductVersion string subclass in types.py with regex-based validation in __new__. - Refactored gitea.py to return ProductVersion | None instead of "". - Moved VERSION_EXTRACT_REGEX to types.py to centralize logic. - Updated extraction logic to use fullmatch for stricter validation. - Updated dependent code to use explicit is not None checks. User Benefits: - Improved type safety and runtime validation of product versions. - Clearer API contracts with Optional types. - Centralized version parsing logic improves maintainability.

okurz added the first-in-queue This is a first PR in a queue of multiple depending on each other label Mar 9, 2026

okurz mentioned this pull request Mar 9, 2026

feat(types): implement ChannelType to centralize channel identification #430

Merged

asmorodskyi requested changes Mar 10, 2026

View reviewed changes

asmorodskyi reviewed Mar 10, 2026

View reviewed changes

Comment thread openqabot/loader/gitea.py Outdated

asmorodskyi reviewed Mar 10, 2026

View reviewed changes

Comment thread openqabot/loader/gitea.py

asmorodskyi reviewed Mar 10, 2026

View reviewed changes

okurz mentioned this pull request Mar 10, 2026

RFC: refactor: move static methods to free functions #432

Open

okurz added 6 commits March 10, 2026 14:07

refactor(types/submission): reduce cyclomatic complexity in __init__

31e7fad

Refactor __init__ into helper methods to reduce CC from 27 to manageable levels.

okurz force-pushed the feature/033_improve_code_quality branch 2 times, most recently from 6c4b6b0 to 5fb9462 Compare March 10, 2026 13:08

Martchus approved these changes Mar 10, 2026

View reviewed changes

asmorodskyi approved these changes Mar 10, 2026

View reviewed changes

mergify bot merged commit 5fb9462 into openSUSE:master Mar 10, 2026
7 checks passed

okurz deleted the feature/033_improve_code_quality branch March 10, 2026 15:45

okurz removed the first-in-queue This is a first PR in a queue of multiple depending on each other label Mar 10, 2026

okurz mentioned this pull request Mar 11, 2026

[WIP][RFC] feat(types): harden product version with validated ProductVersion type #433

Draft

Conversation

okurz commented Mar 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Mar 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

asmorodskyi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asmorodskyi left a comment

Choose a reason for hiding this comment

Uh oh!

mergify bot commented Mar 10, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

okurz commented Mar 9, 2026 •

edited

Loading

codecov bot commented Mar 9, 2026 •

edited

Loading