Skip to content

feat: add cr_checker - #2

Merged
AlexanderLanin merged 22 commits into
eclipse-score:mainfrom
MaximilianSoerenPollak:MSP_add_cr_checker
Jul 21, 2026
Merged

feat: add cr_checker#2
AlexanderLanin merged 22 commits into
eclipse-score:mainfrom
MaximilianSoerenPollak:MSP_add_cr_checker

Conversation

@MaximilianSoerenPollak

Copy link
Copy Markdown
Contributor

Moving cr_checker from the tooling repository.
To start the splitting up process of the tooling repo.

This still has a dependency to tooling as python_basics has not been moved yet.
This will be independed once that is done too.

Comment thread .github/workflows/tests.yml Outdated
Comment thread .github/workflows/tests.yml Outdated
Comment thread cr_checker/resources/exclusion.txt Outdated
Comment thread cr_checker/tool/templates.ini
Comment thread cr_checker/tests/MODULE.bazel Outdated
Comment thread cr_checker/cr_checker.bzl
Comment thread cr_checker/cr_checker.bzl Outdated
Comment thread cr_checker/resources/config.json Outdated
Comment thread LICENSES/Apache-2.0.txt Outdated
Comment thread REUSE.toml Outdated
Comment thread .github/workflows/tests.yml
Comment thread MODULE.bazel Outdated
Comment thread cr_checker/deps.bzl Outdated
Comment thread cr_checker/README.md
Comment thread .pre-commit-hooks.yaml
# *******************************************************************************
- id: copyright
name: Check and fix copyright headers with cr_checker
entry: cr_checker/tool/cr_checker.py --fix

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Does it need the exclusion as well?

Comment thread cr_checker/LICENSE Outdated
@@ -0,0 +1,13 @@
Copyright 2025 Contributors to the Eclipse Foundation

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Remove? As the parent dir has one already?

Comment thread MODULE.bazel Outdated
name = "score_tools"
)
# CR_CHECKER
bazel_dep(name = "aspect_rules_py", version = "1.4.0")

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

See if this can be upgraded?

@AlexanderLanin

This comment was marked as outdated.

@AlexanderLanin

This comment was marked as outdated.

AlexanderLanin

This comment was marked as outdated.

@AlexanderLanin

Copy link
Copy Markdown
Member

Re-review

Re-reviewed after fix: remove not needed LICENCE (6e4236e) and fix: fix Pr comments (caf036c).

✅ Both remaining bugs fixed

  • load_exclusion — now builds a fresh list (appends only valid entries) instead of aliasing and mutating during iteration. The skip-after-removal bug is gone.
  • Macro data — the template/exclusion appends moved outside the for t_name loop, so data is built once. The .fix target no longer gets duplicate labels; bazel run //:copyright-fix is no longer at risk.

✅ Minor items also addressed

  • import json removed from tests/test_cr_checker.py.
  • templates.ini kept in py_library.srcs but now documented with the rationale (avoids runtime path resolution) — reasonable call.
  • Bumped aspect_rules_py 1.4.0 → 1.6.3; dropped the stray cr_checker/LICENSE.

Verified locally: pytest cr_checker/tests/68 passed.


Verdict: All findings from prior reviews are resolved. Nothing blocking remains from my side. 👍

(Only nit left, non-blocking: path = Path(item) in load_exclusion still shadows the path parameter — cosmetic, since path isn't used after the loop.)

@AlexanderLanin AlexanderLanin changed the title Feat: Add cr_checker feat: add cr_checker Jul 20, 2026
AlexanderLanin
AlexanderLanin previously approved these changes Jul 20, 2026
@AlexanderLanin
AlexanderLanin merged commit 7694bf8 into eclipse-score:main Jul 21, 2026
3 checks passed
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