Skip to content

refactor: move parse and related function to common - #337

Merged
furtib merged 6 commits into
Ericsson:mainfrom
furtib:refactor_parse_module
Oct 8, 2026
Merged

furtib merged 6 commits into
Ericsson:mainfrom
furtib:refactor_parse_module

Conversation

@furtib

@furtib furtib commented Sep 22, 2026 •

Copy link
Copy Markdown
Contributor

Why:
We want to have feature and behaviour parity between the monolithic and the per-file rule. To achieve this, we want these modules to share code where it's possible.

Related: #315

What:

  • Moved code related to parsing and severity checking to common.py.

Addresses:
none

Depends on: #342

@furtib furtib self-assigned this Sep 22, 2026
@furtib furtib added the non-functional change ☮️ The patch doesn't change any functionality, e.g. refactoring, documentation, test-only. label Sep 22, 2026
@furtib

furtib commented Sep 22, 2026 •

Copy link
Copy Markdown
Contributor Author

CI fails on unchanged files.
The reason is a module name collision between the common in test/common and the common in src/common.

Since we plan on removing test/common after merging all pytest-to-Bazel PRs (#329 #298 #318), I think we should merge those and, if necessary, change the name of test/common, not this one.

@furtib
furtib marked this pull request as draft September 22, 2026 13:48
@furtib
furtib force-pushed the refactor_parse_module branch from 1266984 to 82816f1 Compare October 5, 2026 12:52
@furtib
furtib marked this pull request as ready for review October 5, 2026 12:52
@furtib
furtib requested a review from Szelethus October 5, 2026 12:55

@Szelethus Szelethus left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I figured a primary motivation behind this patch was unifying per_file and monolithic runs, but I see that you don't call parse at all from per_file. Is this intentional?

@furtib

furtib commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor Author

The primary motivation in this PR was to split the #315 patch into two.
One where I don't change the behaviour of the per-file rule, only refactor (this PR), and another (this will be #315) where I do change the behaviour of the per-file rule.
The per-file rule so far only performs the analysis step, which is not suitable for unification.
Since the per-file rule currently does not perform a parsing step, I figured a non-functional change should not introduce that either.

@furtib
furtib requested a review from Szelethus October 6, 2026 12:49
@furtib
furtib force-pushed the refactor_parse_module branch from 82816f1 to e869f1d Compare October 6, 2026 13:15
@furtib
furtib force-pushed the refactor_parse_module branch from 7f4305c to 060baef Compare October 6, 2026 14:24

@Szelethus Szelethus left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I caught a few things from #345 that would be better placed here. In particular, what about src/per_file.bzl:117, which is also adds "--mode=Run" to the invocation of the per-file script?

Comment thread src/codechecker_script.py Outdated
Comment thread src/common.py Outdated
@furtib
furtib requested a review from Szelethus October 8, 2026 10:37

@Szelethus Szelethus left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM; I'm sure we'll catch something that could've been moved here, but at that point we are making our life harder for no gain. We want this patch, so lets not delay it any longer.

@furtib
furtib merged commit 1b993cb into Ericsson:main Oct 8, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

non-functional change ☮️ The patch doesn't change any functionality, e.g. refactoring, documentation, test-only.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants