Skip to content

refactor(check): rename variable and compile pattern once #1470

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom
Merged
merged 1 commit into from
Jun 8, 2025
Merged

refactor(check): rename variable and compile pattern once #1470

merged 1 commit into from
Jun 8, 2025
+15 −12

Conversation

bearomorphism
Copy link
Contributor

@bearomorphism bearomorphism commented May 30, 2025 •
edited
Loading

Description

Checklist

Code Changes

  • Add test cases to all the changes you introduce
  • Run poetry all locally to ensure this change passes linter check and tests
  • Manually test the changes:
    • Verify the feature/bug fix works as expected in real-world scenarios
    • Test edge cases and error conditions
    • Ensure backward compatibility is maintained
    • Document any manual testing steps performed
  • Update the documentation for the changes

Expected Behavior

Steps to Test This Pull Request

Run cz check --message "<random strings>" to check if the error message works like before.

Additional Context

@bearomorphism bearomorphism force-pushed the refactor-check-2 branch 2 times, most recently from eac0d6f to 37d65a2 Compare May 30, 2025 04:44
@codecov Codecov
Copy link

codecov bot commented May 30, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.87%. Comparing base (a9cd957) to head (c23c261).
Report is 8 commits behind head on refactors.

Additional details and impacted files 
@@            Coverage Diff             @@
##           refactors    #1470   +/-   ##
==========================================
  Coverage      97.87%   97.87%           
==========================================
  Files             57       57           
  Lines           2677     2677           
==========================================
  Hits            2620     2620           
  Misses            57       57
Flag Coverage Δ
unittests 97.87% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@bearomorphism bearomorphism marked this pull request as ready for review May 30, 2025 04:51
Lee-W
Lee-W reviewed May 31, 2025
View reviewed changes
Copy link
Member

@Lee-W Lee-W left a comment

Choose a reason for hiding this comment

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

I'm somewhat worried about this one. I guess there might been users actually use the message to check. Probably could make it a deprecated thing and change in v5

@Lee-W Lee-W added this to the 4.8.3 milestone May 31, 2025
@bearomorphism
Copy link
Contributor Author

I'm somewhat worried about this one. I guess there might been users actually use the message to check. Probably could make it a deprecated thing and change in v5

Then let me revert the capitalized part of this PR.

@bearomorphism bearomorphism requested a review from Lee-W May 31, 2025 13:52
@bearomorphism
Copy link
Contributor Author

I'm somewhat worried about this one. I guess there might been users actually use the message to check. Probably could make it a deprecated thing and change in v5

Fixed. You meant the error message, right?

@bearomorphism bearomorphism changed the title refactor(check): capitalize error message, rename variable refactor(check): rename variable and compile pattern once May 31, 2025
Lee-W
Lee-W reviewed Jun 8, 2025
View reviewed changes
Copy link
Member

@Lee-W Lee-W left a comment

Choose a reason for hiding this comment

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

I'm not sure I get the purpose of this PR after the changes. It seems the pattern only compiles once before this PR. or is there anywhere else I missed?

bearomorphism
bearomorphism commented Jun 8, 2025
View reviewed changes
commitizen/commands/check.py
if self.max_msg_length:
msg_len = len(commit_msg.partition("\n")[0].strip())
if msg_len > self.max_msg_length:
return False
return bool(re.match(pattern, commit_msg))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure I get the purpose of this PR after the changes. It seems the pattern only compiles once before this PR. or is there anywhere else I missed?

@Lee-W Before this change, each self._validate_commit_message evaluates re.match(pattern, _) once, where re.match compiles the input string every time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hence self.cz.schema_pattern() is compiled len(commits) times (assuming no early return)

Copy link
Member

Choose a reason for hiding this comment

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

got it. make sense. let's merge it 🙂

@bearomorphism bearomorphism mentioned this pull request Jun 8, 2025
Open
10 tasks
@Lee-W Lee-W merged commit a272305 into commitizen-tools:refactors Jun 8, 2025
18 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@Lee-W Lee-W Lee-W approved these changes

@woile woile Awaiting requested review from woile woile is a code owner

@noirbizarre noirbizarre Awaiting requested review from noirbizarre noirbizarre is a code owner

Assignees
No one assigned
Labels
pr-status: wait-for-modification pr-status: wait-for-review type: refactor
Projects
None yet
Milestone
4.8.3
Development

Successfully merging this pull request may close these issues.
2 participants
@bearomorphism @Lee-W