Skip to content

Conversation

@BelpHegoR17
Copy link

@BelpHegoR17 BelpHegoR17 commented Jan 1, 2026

Checklist

  • I have read the OpenWISP Contributing Guidelines.
  • I have manually tested the changes proposed in this pull request.
  • I have written new test cases for new code and/or updated existing tests for changes to existing code.
  • I have updated the documentation.

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 commit command. Contributors are prompted step by step to generate a commit message that follows OpenWISP conventions.

The workflow enforces:

  • a consistent commit message structure
  • a properly formatted commit title
  • inclusion of an issue reference in the title
  • automatic generation of a Fixes #<issue_number> footer

The following commit prefixes are supported and encouraged:

[feature], [change], [fix], [docs], [test], [ci], [qa], [chore]

If none of the predefined prefixes apply, contributors can select other and provide a custom prefix . This keeps the workflow flexible while still enforcing a consistent format.

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
@coderabbitai
Copy link

coderabbitai bot commented Jan 1, 2026

Walkthrough

Adds an OpenWisp Commitizen plugin, a top-level module export, a new .cz.toml config, and setup packaging/requirements changes. The plugin enforces bracketed type prefixes (including a custom "other"), capitalized titles that include an issue reference, requires a non-empty body, appends Fixes #<issue> footer, and validates full commit messages.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor Developer as Dev
  participant CLI as "Commitizen CLI"
  participant Plugin as "OpenWispCommitizen"
  participant Repo as "SCM / Repo"

  Dev->>CLI: run commitizen
  CLI->>Plugin: invoke questions()
  Plugin->>Dev: prompt type / custom_prefix? / title / body
  Dev-->>Plugin: answers
  Plugin->>Plugin: message(answers) -> assemble header, body, "Fixes #<issue>"
  Plugin->>Plugin: validate_commit_message(message)
  alt valid
    Plugin->>CLI: return commit message
    CLI->>Repo: create commit with message
    Repo->>Repo: update changelog on bump (if configured)
  else invalid
    Plugin->>CLI: format_error_message -> show errors
    CLI->>Dev: display validation errors
  end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: adding Commitizen integration to standardize commit messages in the QA workflow, directly addressing issue #110.
Description check ✅ Passed The PR description covers the main purpose, enforces workflows, and lists supported prefixes. However, test cases were not updated and documentation was not updated as indicated in the checklist.
Linked Issues check ✅ Passed The implementation fully addresses issue #110 requirements: uses full-word prefixes in square brackets, enforces uppercase after bracket, includes all conventional commit keywords plus [change], provides Commitizen tool integration for standardized workflow.
Out of Scope Changes check ✅ Passed All changes are directly scoped to Commitizen integration: new configuration file, OpenWispCommitizen class, module setup, and dependency declaration. No unrelated changes detected.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e7d3c2c and c789bd0.

📒 Files selected for processing (2)
  • .cz.toml
  • setup.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • setup.py
⏰ 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~=4.2.0
  • GitHub Check: Python==3.11 | django~=5.2.0
  • GitHub Check: Python==3.10 | django~=5.2.0
  • GitHub Check: Python==3.10 | django~=5.1.0
  • GitHub Check: Python==3.13 | django~=5.2.0
  • GitHub Check: Python==3.11 | django~=4.2.0
  • GitHub Check: Python==3.11 | django~=5.1.0
  • GitHub Check: Python==3.13 | django~=5.1.0
  • GitHub Check: Python==3.12 | django~=5.1.0
  • GitHub Check: Python==3.12 | django~=5.2.0
  • GitHub Check: Python==3.10 | django~=5.0.0
  • GitHub Check: Python==3.12 | django~=5.0.0
  • GitHub Check: Python==3.11 | django~=5.0.0
  • GitHub Check: Python==3.10 | django~=4.2.0
🔇 Additional comments (1)
.cz.toml (1)

1-5: Commitizen config is correct and properly configured

The .cz.toml file is correctly formatted. Commitizen version 4.0.0+ (specified in setup.py extras_require) fully supports all four configuration keys:

  • version_provider = "scm" reads versions from git tags
  • tag_format = "v$version" is the standard tag format syntax
  • update_changelog_on_bump = true enables automatic changelog generation
  • name = "openwisp" correctly references the custom plugin registered in setup.py entry_points

The CHANGES.rst changelog file exists with proper reStructuredText structure. Configuration is production-ready.


Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

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.ClassVar to 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 the message parameter with an underscore.

The message parameter is required by the BaseCommitizen.format_error_message interface but is currently unused. Since the method intentionally returns a static error message regardless of input, prefix the parameter with _message to explicitly mark it as intentionally unused and comply with the interface contract.

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 761112c and ed7b491.

📒 Files selected for processing (4)
  • .cz.toml
  • cz_openwisp.py
  • openwisp_utils/commitizen/openwisp.py
  • setup.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
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

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 that match is not None. While _validate_title should 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_PREFIXES should be annotated with typing.ClassVar to 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 message parameter 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

📥 Commits

Reviewing files that changed from the base of the PR and between b90f80a and 5c1f8f7.

📒 Files selected for processing (2)
  • openwisp_utils/commitizen/openwisp.py
  • setup.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_modules entry correctly references the top-level cz_openwisp.py module, enabling the commitizen plugin entry point to import OpenWispCommitizen.


24-27: LGTM!

The commitizen plugin entry point is correctly configured. The mapping enables users to use the custom plugin via cz commit with the name = "openwisp" configuration in .cz.toml.


38-38: LGTM! Version constraint addresses past concern.

The version constraint >=4.0.0,<5.0.0 correctly 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_prefix prompt provides flexibility while maintaining structure. The implementation is now consistent with the documented format shown in example() and info().


146-178: LGTM! Past issues resolved.

The documentation methods are now internally consistent:

  • example() demonstrates the actual commit format
  • schema_pattern() correctly excludes "other" from the validation pattern (since it's only an interactive option)
  • info() provides clear guidance on the convention

These align well with the implementation in questions() and message().


180-181: LGTM!

The initialization correctly delegates to the parent BaseCommitizen class, following the standard Commitizen plugin pattern.

Fixed the error shown due to flake8

Fixes openwisp#110
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

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.ClassVar to 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_title ensures 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 an AttributeError.

🔎 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 message parameter is unused. If this is required by the BaseCommitizen interface, 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

📥 Commits

Reviewing files that changed from the base of the PR and between 5c1f8f7 and f75b7a4.

📒 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.

@coveralls
Copy link

coveralls commented Jan 1, 2026

Coverage Status

coverage: 97.253%. remained the same
when pulling c789bd0 on BelpHegoR17:issues/110-commit-convention-standardization
into 761112c on openwisp:master.

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
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
openwisp_utils/commitizen/openwisp.py (1)

14-25: Add ClassVar annotation for mutable class attribute.

ALLOWED_PREFIXES is a mutable class attribute and should be annotated with typing.ClassVar to 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

📥 Commits

Reviewing files that changed from the base of the PR and between f75b7a4 and c141917.

📒 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_prefix and _validate_title provide 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 message parameter, but this is likely required by the BaseCommitizen interface and is acceptable.


149-181: LGTM: Documentation methods provide clear guidance.

The example(), schema(), schema_pattern(), and info() methods consistently document the OpenWISP commit convention. The schema_pattern correctly 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
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (2)
openwisp_utils/commitizen/openwisp.py (2)

15-25: Consider adding ClassVar annotation for type safety.

The ALLOWED_PREFIXES class attribute would benefit from a typing.ClassVar annotation 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

📥 Commits

Reviewing files that changed from the base of the PR and between c141917 and e7d3c2c.

📒 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_RE and _HEADER_RE has 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(), and info() methods provide consistent, comprehensive information about OpenWISP commit conventions. The schema_pattern() correctly excludes "other" from valid commit types.


183-184: LGTM! Proper initialization.

Correctly delegates to the parent class constructor.


136-147: Verify if the message parameter is required by the BaseCommitizen interface.

Static analysis flags the message parameter as unused (ARG002). Since this method overrides BaseCommitizen.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.

Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

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",
Copy link
Member

Choose a reason for hiding this comment

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

Move to the qa section.

.cz.toml Outdated
name = "openwisp"
version_provider = "scm"
tag_format = "v$version"
update_changelog_on_bump = true No newline at end of file
Copy link
Member

Choose a reason for hiding this comment

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

Add a blank line at the end of this file please.

commitizen has been moved to the qa optional dependencies.

Fixes openwisp#110
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.

[qa] Enforce conventional commit style

3 participants