[feature/qa] Standardize commit messages using commitizen #110 #560
Conversation
This change integrates Commitizen into openwisp_utils to standardize how commit messages are written across the project. It introduces an interactive commit workflow that guides contributors to use the correct OpenWISP commit format, ensures commit titles are properly structured, and enforces the presence of an issue preference. The commit message footer is generated automatically using the provided issue number, improving consistency and making commits easier to review and track. Fixes openwisp#110
WalkthroughAdds Commitizen-based enforcement of OpenWISP commit conventions: new .cz.toml config, a custom Commitizen plugin OpenWispCommitizen (openwisp_utils/commitizen/openwisp.py) with prompts, validation, formatting and help text, a re-export module cz_openwisp.py and packaging entry point, updated QA script (openwisp-qa-check) to call Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as "Commitizen CLI"
participant Plugin as "OpenWispCommitizen"
participant Validator as "Title/Footer Validator"
User->>CLI: cz commit
CLI->>Plugin: questions()
Plugin-->>CLI: prompts (change_type, title, description)
User->>CLI: answers
CLI->>Plugin: message(answers)
Plugin->>Validator: _validate_title(title) / extract issue
Validator-->>Plugin: ok / error
Plugin-->>CLI: formatted commit message or error guidance
CLI->>User: show commit / error guidance
sequenceDiagram
actor Developer
participant Git as "git"
participant QAScript as "openwisp-qa-check"
participant CZ as "Commitizen (cz)"
participant Plugin as "OpenWispCommitizen"
Developer->>Git: git commit
Git->>QAScript: run openwisp-qa-check
QAScript->>QAScript: check `cz` installed
alt cz missing
QAScript-->>Developer: error, set FAILURE
else cz present
QAScript->>CZ: cz check --message "$COMMIT_MESSAGE"
CZ->>Plugin: validate commit message
alt valid
Plugin-->>CZ: pass
CZ-->>QAScript: exit 0
QAScript-->>Developer: success
else invalid
Plugin-->>CZ: fail + error text
CZ-->>QAScript: exit 1
QAScript-->>Developer: error + remediation (cz info, cz example, reset/amend)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
✏️ Tip: You can disable this entire section by setting Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (4)
openwisp_utils/commitizen/openwisp.py (4)
4-4: Clarify the character range in the regex.The character class
[a-z0-9!:-]uses a range!:-which matches ASCII characters from!(33) to:(58), including various special characters like",#,$, etc. If the intent is to only allow specific punctuation, consider using an explicit list like[a-z0-9!:_-]for clarity.🔎 Suggested improvement for explicitness
-_CUSTOM_PREFIX_RE = re.compile(r"^[a-z0-9!:-]+$") +_CUSTOM_PREFIX_RE = re.compile(r"^[a-z0-9!:_-]+$")
16-26: Add ClassVar annotation for the class attribute.The static analysis correctly identifies that this mutable class attribute should be annotated with
typing.ClassVarto indicate it's a class-level constant.🔎 Proposed fix
+from typing import ClassVar + class OpenWispCommitizen(BaseCommitizen): """ Commitizen plugin for OpenWISP commit conventions. """ # Single source for allowed prefixes - ALLOWED_PREFIXES = [ + ALLOWED_PREFIXES: ClassVar[list[str]] = [ "feature", "change", "fix", "docs", "test", "ci", "chore", "qa", "other", ]
36-44: Simplify the string concatenation.Line 43 has an unusual string concatenation with a space between the two string literals. While syntactically correct, it's clearer to use a single string.
🔎 Proposed improvement
if not _TITLE_ISSUE_RE.search(value): - return "Please add the issue number at the end of the title " "(e.g. #104)." + return "Please add the issue number at the end of the title (e.g. #104)." return True
138-149: Prefix themessageparameter with an underscore.The
messageparameter is required by theBaseCommitizen.format_error_messageinterface but is currently unused. Since the method intentionally returns a static error message regardless of input, prefix the parameter with_messageto explicitly mark it as intentionally unused and comply with the interface contract.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.cz.tomlcz_openwisp.pyopenwisp_utils/commitizen/openwisp.pysetup.py
🧰 Additional context used
🧬 Code graph analysis (1)
cz_openwisp.py (1)
openwisp_utils/commitizen/openwisp.py (1)
OpenWispCommitizen(10-187)
🪛 Ruff (0.14.10)
openwisp_utils/commitizen/openwisp.py
16-26: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
138-138: Unused method argument: message
(ARG002)
🔇 Additional comments (5)
.cz.toml (1)
1-5: LGTM! Configuration is well-structured.The Commitizen configuration properly sets up the OpenWISP plugin with SCM version provider and changelog generation on version bumps.
cz_openwisp.py (1)
1-3: LGTM! Clean re-export module.The module correctly exposes the OpenWispCommitizen plugin for Commitizen's plugin system.
setup.py (2)
18-18: Module registration looks correct.The
py_modules=["cz_openwisp"]properly registers the standalone module for the plugin system.
25-27: Plugin entry point is correctly configured.The commitizen.plugin entry point properly wires the OpenWispCommitizen class to the "openwisp" plugin name.
openwisp_utils/commitizen/openwisp.py (1)
186-187: LGTM! Constructor correctly initializes the base class.
This commit fixes issues discovered after opening the PR, including updating the Commitizen example and info output, pinning the Commitizen dependency to a compatible version range, and correcting minor configuration problems. Fixes openwisp#110
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
openwisp_utils/commitizen/openwisp.py (2)
95-131: Update docstring to match actual validation behavior.The docstring claims "Why and How sections must be present and non-empty," but the validation only checks that the body is non-empty (lines 117-119). It doesn't enforce any "Why:" or "How:" section structure. Update the docstring to accurately describe what the method validates.
🔎 Proposed fix
def validate_commit_message(self, message: str) -> bool: - """Enforce OpenWISP commit rules: - Title must start with a capital letter - Why and How sections must be present and non-empty""" + """ + Enforce OpenWISP commit rules: + - Header must match the format: [prefix] Capitalized title #<issue> + - Body must be present and non-empty + - Footer must contain "Fixes #<issue>" + - Issue number in title and footer must match + """
79-93: Add defensive null check for regex match.Line 91 directly accesses
match.group(1)without verifying thatmatchis notNone. While_validate_titleshould prevent this scenario, defensive programming requires a null check to handle edge cases where validation might be bypassed or fail.🔎 Proposed fix
def message(self, answers): if answers["change_type"] == "other": prefix_value = answers["custom_prefix"] else: prefix_value = answers["change_type"] prefix = f"[{prefix_value}]" title = answers["title"].strip() body = answers["how"].strip() # Extract issue number from title match = _TITLE_ISSUE_RE.search(title) + if not match: + raise ValueError( + "Title must contain an issue number (e.g., #123). " + "This should have been caught by validation." + ) issue_number = match.group(1) return f"{prefix} {title}\n\n" f"{body}\n\n" f"Fixes #{issue_number}"
🧹 Nitpick comments (2)
openwisp_utils/commitizen/openwisp.py (2)
15-25: Consider adding ClassVar annotation.The mutable class attribute
ALLOWED_PREFIXESshould be annotated withtyping.ClassVarto clarify it's a class-level constant shared across instances.🔎 Proposed refactor
+from typing import ClassVar + import re from commitizen.cz.base import BaseCommitizen _CUSTOM_PREFIX_RE = re.compile(r"^[a-z0-9!:-]+$") _TITLE_ISSUE_RE = re.compile(r"#(\d+)$") _HEADER_RE = re.compile(r"^\[[a-z0-9:!-]+\] [A-Z].*\s#\d+$") _FIXES_RE = re.compile(r"^Fixes #(\d+)$", re.MULTILINE) class OpenWispCommitizen(BaseCommitizen): """Commitizen plugin for OpenWISP commit conventions.""" # Single source for allowed prefixes - ALLOWED_PREFIXES = [ + ALLOWED_PREFIXES: ClassVar[list[str]] = [ "feature", "change", "fix", "docs", "test", "ci", "chore", "qa", "other", ]Based on static analysis.
133-144: Remove unused parameter.The
messageparameter is declared but never used in the method body. Consider removing it since the method returns a static error message.🔎 Proposed fix
- def format_error_message(self, message: str) -> str: + def format_error_message(self) -> str: return ( "Invalid commit message.\n\n" "Expected format:\n\n" "[prefix] Title starting with capital letter #<issue>\n\n" "<commit body>\n\n" "Fixes #<issue>\n\n" "Example:\n" "[feature] Add subnet import support #104\n\n" "Add support for importing multiple subnets from a CSV file.\n\n" "Fixes #104" )Based on static analysis.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
openwisp_utils/commitizen/openwisp.pysetup.py
🧰 Additional context used
🪛 Ruff (0.14.10)
openwisp_utils/commitizen/openwisp.py
15-25: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
133-133: Unused method argument: message
(ARG002)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.0.0
🔇 Additional comments (9)
setup.py (3)
18-18: LGTM!The
py_modulesentry correctly references the top-levelcz_openwisp.pymodule, enabling the commitizen plugin entry point to importOpenWispCommitizen.
24-27: LGTM!The commitizen plugin entry point is correctly configured. The mapping enables users to use the custom plugin via
cz commitwith thename = "openwisp"configuration in.cz.toml.
38-38: LGTM! Version constraint addresses past concern.The version constraint
>=4.0.0,<5.0.0correctly locks the dependency to the 4.x series, preventing potential breakage from future BaseCommitizen API changes. This addresses the concern raised in the previous review.openwisp_utils/commitizen/openwisp.py (6)
1-9: LGTM!The imports and regex patterns are well-defined for validating the OpenWISP commit convention. The patterns correctly enforce the bracketed prefix format, issue number extraction, and Fixes footer validation.
27-33: LGTM! Past issue resolved.The validation correctly enforces non-empty, lowercase custom prefixes matching the allowed pattern. The trailing spaces issue from the previous review has been fixed.
35-43: LGTM!The title validation correctly enforces the OpenWISP convention: non-empty, capitalized first letter, and mandatory issue reference at the end (e.g.,
#104).
45-77: LGTM!The interactive questions are well-structured with appropriate validation for each field. The conditional
custom_prefixprompt provides flexibility while maintaining structure. The implementation is now consistent with the documented format shown inexample()andinfo().
146-178: LGTM! Past issues resolved.The documentation methods are now internally consistent:
example()demonstrates the actual commit formatschema_pattern()correctly excludes "other" from the validation pattern (since it's only an interactive option)info()provides clear guidance on the conventionThese align well with the implementation in
questions()andmessage().
180-181: LGTM!The initialization correctly delegates to the parent
BaseCommitizenclass, following the standard Commitizen plugin pattern.
Fixed the error shown due to flake8 Fixes openwisp#110
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
openwisp_utils/commitizen/openwisp.py (5)
15-25: Add ClassVar annotation for mutable class attribute.As flagged by static analysis, mutable class attributes should be annotated with
typing.ClassVarto clearly indicate they are class-level variables rather than instance attributes.🔎 Proposed fix
+from typing import ClassVar + from commitizen.cz.base import BaseCommitizen _CUSTOM_PREFIX_RE = re.compile(r"^[a-z0-9!:-]+$")"""Commitizen plugin for OpenWISP commit conventions.""" # Single source for allowed prefixes - ALLOWED_PREFIXES = [ + ALLOWED_PREFIXES: ClassVar[list[str]] = [ "feature", "change", "fix",
27-33: Improve error message clarity.The error message at line 32 only states "Prefix must be lowercase" but doesn't explain which characters are allowed. According to
_CUSTOM_PREFIX_RE, the prefix must be lowercase alphanumeric with optional punctuation (:,!,-).🔎 Proposed fix
if not _CUSTOM_PREFIX_RE.match(value): - return "Prefix must be lowercase." + return "Prefix must be lowercase and may only contain letters, digits, or punctuation (: ! -)." return True
35-43: Minor: Fix string formatting.Line 42 has awkward string concatenation with an unnecessary space before the parenthetical example.
🔎 Proposed fix
if not _TITLE_ISSUE_RE.search(value): - return "Please add the issue number at the end of the title " "(e.g. #104)." + return "Please add the issue number at the end of the title (e.g., #104)." return True
79-93: Consider defensive null check for regex match.While the validation in
_validate_titleensures an issue number exists, defensive programming suggests adding a null check for the regex match result. If validation is bypassed or the regex pattern changes, line 91 would raise anAttributeError.🔎 Proposed fix
# Extract issue number from title match = _TITLE_ISSUE_RE.search(title) + if not match: + raise ValueError("Title validation failed: missing issue number") issue_number = match.group(1)
132-143: Mark unused parameter with underscore prefix.As flagged by static analysis, the
messageparameter is unused. If this is required by theBaseCommitizeninterface, prefix it with an underscore to indicate it's intentionally unused.🔎 Proposed fix
- def format_error_message(self, message: str) -> str: + def format_error_message(self, _message: str) -> str: return ( "Invalid commit message.\n\n" "Expected format:\n\n"
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
openwisp_utils/commitizen/openwisp.py
🧰 Additional context used
🪛 Ruff (0.14.10)
openwisp_utils/commitizen/openwisp.py
15-25: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
132-132: Unused method argument: message
(ARG002)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.0.0
- GitHub Check: Python==3.10 | django~=4.2.0
🔇 Additional comments (5)
openwisp_utils/commitizen/openwisp.py (5)
1-9: LGTM!The imports and regex constants are well-defined and appropriate for the validation requirements.
45-77: LGTM!The interactive question flow is well-structured with appropriate validations and conditional logic for custom prefixes.
95-130: LGTM!The validation logic is comprehensive and correctly enforces all required commit message constraints: proper header format, non-empty body, presence of Fixes footer, and consistent issue number references.
145-177: LGTM!The documentation methods are well-aligned with the implementation and provide clear guidance on the commit message format and allowed types.
179-180: LGTM!Standard initialization correctly delegates to the parent class.
While the issue number is already enforced earlier in the workflow, I’ve added a defensive check when extracting it from the commit title to avoid relying solely on upstream validation and prevent unexpected errors in future refactors. Fixes openwisp#110
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
openwisp_utils/commitizen/openwisp.py (1)
14-25: Add ClassVar annotation for mutable class attribute.
ALLOWED_PREFIXESis a mutable class attribute and should be annotated withtyping.ClassVarto clarify it's shared across all instances and follows Python typing best practices.🔎 Proposed fix
+from typing import ClassVar import re from commitizen.cz.base import BaseCommitizen _CUSTOM_PREFIX_RE = re.compile(r"^[a-z0-9!:-]+$") _TITLE_ISSUE_RE = re.compile(r"#(\d+)$") _HEADER_RE = re.compile(r"^\[[a-z0-9:!-]+\] [A-Z].*\s#\d+$") _FIXES_RE = re.compile(r"^Fixes #(\d+)$", re.MULTILINE) class OpenWispCommitizen(BaseCommitizen): """Commitizen plugin for OpenWISP commit conventions.""" # Single source for allowed prefixes - ALLOWED_PREFIXES = [ + ALLOWED_PREFIXES: ClassVar[list[str]] = [ "feature", "change", "fix", "docs", "test", "ci", "chore", "qa", "other", ]Based on static analysis hints.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
openwisp_utils/commitizen/openwisp.py
🧰 Additional context used
🪛 Ruff (0.14.10)
openwisp_utils/commitizen/openwisp.py
15-25: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
92-94: Avoid specifying long messages outside the exception class
(TRY003)
136-136: Unused method argument: message
(ARG002)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.10 | django~=5.0.0
- GitHub Check: Python==3.10 | django~=5.2.0
🔇 Additional comments (7)
openwisp_utils/commitizen/openwisp.py (7)
27-43: LGTM: Validation methods are well-implemented.Both
_validate_custom_prefixand_validate_titleprovide clear validation with helpful error messages. The logic correctly enforces the OpenWISP commit conventions.
45-77: LGTM: Interactive prompts align with documented format.The
questions()method provides a clear, guided workflow. The conditional custom prefix prompt (when "other" is selected) and validation callbacks ensure users create properly formatted commits.
79-97: LGTM: Message assembly includes proper null check.The defensive null check (lines 91-95) prevents potential AttributeError and provides a clear error message. The message assembly correctly formats the commit with prefix, title, body, and Fixes footer.
Note: Static analysis suggests extracting the exception message to a constant, but for a single use case this is an optional refactor.
99-134: LGTM: Comprehensive validation logic.The validation correctly enforces all OpenWISP commit conventions: proper header format, non-empty body, required Fixes footer, and consistent issue number references between title and footer. The logic flow is clear and thorough.
136-147: LGTM: Clear error formatting.The error message provides a clear explanation of the expected format with a concrete example.
Note: Static analysis flags the unused
messageparameter, but this is likely required by theBaseCommitizeninterface and is acceptable.
149-181: LGTM: Documentation methods provide clear guidance.The
example(),schema(),schema_pattern(), andinfo()methods consistently document the OpenWISP commit convention. Theschema_patterncorrectly excludes "other" as an actual commit type while allowing the predefined types.
183-184: LGTM: Proper initialization.
Ensure both interactive and final validation require a space before the issue number to avoid false positives. Fixes openwisp#110
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
openwisp_utils/commitizen/openwisp.py (2)
15-25: Consider addingClassVarannotation for type safety.The
ALLOWED_PREFIXESclass attribute would benefit from atyping.ClassVarannotation to clearly indicate it's a class-level constant and improve type checking.🔎 Proposed refactor
+from typing import ClassVar + from commitizen.cz.base import BaseCommitizen _CUSTOM_PREFIX_RE = re.compile(r"^[a-z0-9!:-]+$")"""Commitizen plugin for OpenWISP commit conventions.""" # Single source for allowed prefixes - ALLOWED_PREFIXES = [ + ALLOWED_PREFIXES: ClassVar[list[str]] = [ "feature", "change",Based on static analysis hint RUF012.
79-97: Good defensive programming with the null check.The message assembly correctly handles all cases and includes a defensive check for missing issue numbers.
The inline exception message at lines 92-94 could be moved to a constant for consistency with style guides, though this is a minor style preference.
🔎 Optional refactor
+_TITLE_ISSUE_MISSING_ERROR = ( + "Commit title must end with an issue reference like #<issue_number>." +) + + class OpenWispCommitizen(BaseCommitizen):# Extract issue number from title match = _TITLE_ISSUE_RE.search(title) if not match: - raise ValueError( - "Commit title must end with an issue reference like #<issue_number>." - ) + raise ValueError(_TITLE_ISSUE_MISSING_ERROR) issue_number = match.group(1)Based on static analysis hint TRY003.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
openwisp_utils/commitizen/openwisp.py
🧰 Additional context used
🪛 Ruff (0.14.10)
openwisp_utils/commitizen/openwisp.py
15-25: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
92-94: Avoid specifying long messages outside the exception class
(TRY003)
136-136: Unused method argument: message
(ARG002)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.0.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
🔇 Additional comments (7)
openwisp_utils/commitizen/openwisp.py (7)
1-9: LGTM! Regex patterns are well-defined and consistent.The patterns correctly enforce OpenWISP commit conventions, and the previous inconsistency between
_TITLE_ISSUE_REand_HEADER_REhas been resolved—both now require a space before the issue number.
27-43: LGTM! Validation methods are well-implemented.Both validators correctly enforce the OpenWISP commit conventions with clear, helpful error messages.
45-77: LGTM! Interactive prompts are well-structured.The question flow correctly guides users through the commit creation process, with appropriate validation at each step and conditional custom prefix input.
99-134: LGTM! Comprehensive validation logic.The validation correctly enforces all OpenWISP commit conventions, including matching issue numbers between the title and footer, non-empty body, and proper header format.
149-181: LGTM! Helper methods provide clear guidance.The
example(),schema(),schema_pattern(), andinfo()methods provide consistent, comprehensive information about OpenWISP commit conventions. Theschema_pattern()correctly excludes "other" from valid commit types.
183-184: LGTM! Proper initialization.Correctly delegates to the parent class constructor.
136-147: Verify if themessageparameter is required by theBaseCommitizeninterface.Static analysis flags the
messageparameter as unused (ARG002). Since this method overridesBaseCommitizen.format_error_message, the parameter signature must match the parent class. If the base class interface requires it, the parameter can be safely ignored; if not, consider removing it.
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
This is definitely interesting! We're being flooded with PRs but I am doing my best to review most of them. I will come back to this asap, in the meantime see my comments below.
setup.py
Outdated
| "swapper~=1.4.0", | ||
| # allow wider range here to avoid interfering with other modules | ||
| "urllib3>=2.0.0,<3.0.0", | ||
| "commitizen>=4.0.0,<5.0.0", |
.cz.toml
Outdated
| name = "openwisp" | ||
| version_provider = "scm" | ||
| tag_format = "v$version" | ||
| update_changelog_on_bump = true No newline at end of file |
There was a problem hiding this comment.
Add a blank line at the end of this file please.
commitizen has been moved to the qa optional dependencies. Fixes openwisp#110
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
So how is this thing supposed to be used? Is it a tool to help contributors write compliant commit messages? Is it also able to enforce this or is it meant to be used just for writing?
|
It is currently used only for guided commit creation via Commitizen also provides a |
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
Sorry for my late response, we've been flooded with pull requests and it's been hard to reply to everyone. The PR is interesting but it's missing the following:
- Tests: if someone breaks aspects of the feature inadvertently in the future, we may not find out while reviewing.
- Enforcement: we need to start enforcing this from the start or it will be useless:
- openwisp-qa-check should check the commit message follows the guidelines: if not, it should print an error and suggest a fix (eg: suggest to run a command that will amend the commit message in a way that follows our guidelines)
- Docs: how to use this?
Let us know if you want to follow up and complete it, thanks 🙏
| lines = message.splitlines() | ||
| if not lines: | ||
| return False | ||
|
|
There was a problem hiding this comment.
this method has too many blank lines, please reduce the overall amount of blank lines in the entire code, there's too many and they don't add any benefit, they only make the code longer to scroll. Use blank lines sparingly.
| name = "openwisp" | ||
| version_provider = "scm" | ||
| tag_format = "v$version" | ||
| update_changelog_on_bump = true |
There was a problem hiding this comment.
This file is Commitizen configuration entry point. It tells about custom Commitizen plugin.
| ) | ||
|
|
||
| def __init__(self, config): | ||
| super().__init__(config) |
There was a problem hiding this comment.
It's not needed, will be updated in the PR.
|
See also #523, if we get the work in this PR right we'll definitely use it as a base for that issue. |
|
Thanks for the feedback. I will start working on the missing aspects. |
github-project-automation
bot
moved this to In progress
in OpenWISP Priorities for next releases
nemesifier
changed the title
|
I have implemented the points you highlighted earlier except docs. For the documentation part, I’m unsure about where i should write them. The main contributing guidelines live in |
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
Regarding docs, a short mention and usage example in https://github.com/openwisp/openwisp-utils/blob/master/docs/developer/qa-checks.rst would work.
Questions the docs should answer:
- Why it's there
- How to use it
- How to check if the commit message respects the conventions?
Can we add basic tests for this?
If you look in our existing QA tools, you'll see we already have a commit message check. We shall aim to add the usage of this tool to perform an extra validation step, or alternatively, if this tool can cover all the functionality of the old check, we can just change commitcheck to use this tool and drop the old logic.
Replaces the existing commit message check with Commitizen. Updates the QA enforcement to validate commit messages via Commitizen. Fixes openwisp#110
Remove leading underscore from :ref: target Fixes openwisp#110
Handled merge commits and other corner cases Fixes openwisp#110
Info method now returns the prefixes from the python list Improved error message Removed "chore" in favour of "chores"
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@openwisp_utils/commitizen/openwisp.py`:
- Around line 9-25: ALLOWED_PREFIXES in class OpenWispCommitizen contains
"chores" (plural) while PR objectives and schema use "chore" (singular), causing
inconsistency between interactive prompts and validation; update the
ALLOWED_PREFIXES list in OpenWispCommitizen to replace "chores" with "chore" so
the offered options from cz commit match the documented/validated prefix and
keep the rest of the list intact.
- Around line 46-78: The questions() interactive choices never include an
"other" option so the custom-prefix flow is unreachable; either restore an
"other" choice into the choices list produced in questions() (so the
"custom_prefix" question's when lambda and _validate_custom_prefix are reachable
and the "other" branch in message() is exercised) or remove the dead code paths
entirely: delete the "custom_prefix" question block in questions(), remove the
_validate_custom_prefix method, and remove the "other" branch in message();
locate these by the questions() method, the _validate_custom_prefix method, and
the message() method/class to apply the chosen fix.
- Around line 97-116: In validate_commit_message, remove the early unconditional
acceptance via any(map(commit_msg.startswith, allowed_prefixes)) so prefixes
cannot bypass pattern checks; instead require pattern.match(commit_msg) first,
then (if allowed_prefixes non-empty) verify the commit_msg starts with one of
allowed_prefixes, and then enforce max_msg_length (if not None) before returning
ValidationResult(True, []); use the existing symbols validate_commit_message,
allowed_prefixes, pattern, max_msg_length and ValidationResult to locate and
implement the reordered checks.
- Around line 128-154: Extract the duplicated error text into a class-level
constant (e.g., ERROR_TEMPLATE) and have both validate_commit_message (the
branch that returns ValidationResult(False, [...])) and
format_error_message(message: str) return/use that constant instead of inline
strings; while extracting, change the first sentence to "Invalid commit message
format" and change the "Example:" heading to "Examples:" so the text matches the
test expectations in test_error_message_is_user_friendly, then replace both in
validate_commit_message and format_error_message to reference ERROR_TEMPLATE.
- Around line 167-169: The regex returned by schema_pattern() uses (?sm) and a
trailing $ which, combined with pattern.match in validate_commit_message, lets
header-only matches pass; change the end-anchor in schema_pattern() from $ to \Z
so it truly anchors to end-of-string and update validate_commit_message to call
pattern.fullmatch(commit_msg) instead of pattern.match(commit_msg) for
defense-in-depth; after making these changes, run and adjust tests that assert
outdated error-message substrings in tests.py to match the new validation
behavior.
In `@openwisp_utils/commitizen/tests.py`:
- Around line 93-113: The test_info_includes_all_prefixes test incorrectly
asserts "- chore" which passes because "- chores" contains that substring;
update the test to assert the exact prefix present (replace or remove the "-
chore" assertion and ensure it checks "- chores" instead) or change the
verification to match full prefixes exactly (e.g., parse result.stdout lines and
assert ALLOWED_PREFIXES or the exact string "- chores" is present) so the test
aligns with ALLOWED_PREFIXES and no longer relies on substring matches.
- Around line 44-90: Each test currently compares the 3-tuple returned by
run_cz_check(...) to 0, which is always true; update each failing test (e.g.,
test_empty_commit_message, test_invalid_prefix_format,
test_title_not_capitalized, test_valid_commit_without_issue_number,
test_issue_number_not_at_end_of_title, test_missing_blank_line_after_header,
test_missing_blank_line_before_footer, test_missing_fixes_footer,
test_footer_not_last_line, test_mismatched_issue_numbers) to destructure
run_cz_check into (returncode, stdout, stderr) and assert returncode != 0
(optionally assert expected stderr/stdout content), ensuring the tests actually
check the process exit code.
🧹 Nitpick comments (1)
runtests.py (1)
20-27:os.path.joinwith pre-formed slash-separated strings doesn't add value.
os.path.join("openwisp_utils/releaser/tests")is equivalent to a plain string — the join does nothing when given a single argument containing slashes. Either split into path components or use the string directly.♻️ Suggested change
sys.exit( pytest.main( [ - os.path.join("openwisp_utils/releaser/tests"), - os.path.join("openwisp_utils/commitizen/tests.py"), + os.path.join("openwisp_utils", "releaser", "tests"), + os.path.join("openwisp_utils", "commitizen", "tests.py"), ] ) )
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
openwisp-qa-checkopenwisp_utils/commitizen/openwisp.pyopenwisp_utils/commitizen/tests.pyruntests.py
🧰 Additional context used
🧬 Code graph analysis (1)
openwisp_utils/commitizen/tests.py (1)
openwisp_utils/commitizen/openwisp.py (1)
message(80-95)
🪛 Ruff (0.14.14)
openwisp_utils/commitizen/tests.py
[error] 5-5: subprocess call: check for execution of untrusted input
(S603)
[error] 6-6: Starting a process with a partial executable path
(S607)
[warning] 16-16: Unpacked variable out is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
[warning] 16-16: Unpacked variable err is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
[warning] 22-22: Unpacked variable out is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
[warning] 22-22: Unpacked variable err is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
[warning] 28-28: Unpacked variable out is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
[warning] 28-28: Unpacked variable err is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
[warning] 34-34: Unpacked variable out is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
[warning] 34-34: Unpacked variable err is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
[warning] 40-40: Unpacked variable out is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
[warning] 40-40: Unpacked variable err is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
[error] 95-95: Starting a process with a partial executable path
(S607)
openwisp_utils/commitizen/openwisp.py
[warning] 13-25: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
[warning] 91-93: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 105-105: Unused method argument: commit_hash
(ARG002)
[warning] 143-143: Unused method argument: message
(ARG002)
[warning] 169-169: Unused noqa directive (non-enabled: E501)
Remove unused noqa directive
(RUF100)
🔇 Additional comments (5)
openwisp_utils/commitizen/tests.py (2)
116-128: LGTM — error-message assertions look correct.The test properly validates that user-friendly guidance is shown and that raw regex patterns are not exposed to the end user.
14-41: Valid-message tests look good.These tests correctly destructure the return value and assert on
code == 0.Minor nit per Ruff RUF059:
outanderrare unused in lines 16, 22, 28, 34, 40. Prefixing with underscores (_out,_error_, _) would silence the lint warning, but this is purely cosmetic.openwisp_utils/commitizen/openwisp.py (2)
1-7: LGTM — regex definitions look correct.
_CUSTOM_PREFIX_REand_TITLE_ISSUE_REare well-formed. The earlier review concern about inconsistent space-before-issue was addressed (\#(\d+)$now requires a space before#).
156-165: LGTM —example()andschema()are clear.The example demonstrates the expected commit format, and the schema provides a concise template.
openwisp-qa-check (1)
149-175: LGTM —runcheckcommit()properly usescz check --message.The function correctly checks for
czavailability, falls back togit logfor the commit message, invokescz check --message, and provides actionable guidance on failure (reset + recommit, amend,cz info,cz example). The past review concern about the unused--messageCLI flag is now resolved.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
nemesifier
force-pushed
the
issues/110-commit-convention-standardization
branch
from
e97f368 to
0b91284
Compare
nemesifier
force-pushed
the
issues/110-commit-convention-standardization
branch
from
0b91284 to
7b20dbd
Compare
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
Great work @Stoiicc, I am really glad you took on this issue that has been opened for a long time and I neglected it as well, you motivivated me to invest a bit of time in this and I hope it will help newcomers passing QA checks faster.
I did some changes to make sure all cases are covered, there may be some tweaks needed down the road but we can always open new PRs to fix any issue we find, so it looks good to me now, but please double check my changes, let me know if you have any doubt/question.
github-project-automation
bot
moved this from In progress
to Reviewer approved
in OpenWISP Priorities for next releases
|
I've tested the new code and found that, since CI is running with commitizen 4.13.5, and the implementation works correctly there . In environments using 4.10.x, ValidationResult is not available which causes the import error I saw locally. I think we should update the dependency constraint to commitizen>=4.13.0,<5.0.0 so local environments match CI behavior . |
nemesifier
dismissed stale reviews from coderabbitai[bot] and themself
via
6f0fc27
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
We should have used this tool sooner, we dould avoided a lot of custom additions to openwisp-utils. Thanks for taking the time to do this @Stoiicc 👍
If you want to keep helping us with this sort of release management tasks, there's other similar issues open.
github-project-automation
bot
moved this from In progress
to Done
in OpenWISP Contributor's Board
github-project-automation
bot
moved this from Reviewer approved
to Done
in OpenWISP Priorities for next releases
3 participants
Checklist
Reference to Existing Issue
Closes #110.
Description of Changes
This PR introduces a standardized commit message workflow for OpenWISP by integrating Commitizen into openwisp-utils. The goal is to make commit messages more consistent, easier to review, and simpler for contributors to write correctly. This change adds a guided, interactive commit workflow using the
cz commitcommand. Contributors are prompted step by step to generate a commit message that follows OpenWISP conventions.The workflow enforces:
Allowed commit prefixes:
If none of the predefined prefixes apply, contributors can select "chores".