-
Notifications
You must be signed in to change notification settings - Fork 33
Add support for exempting i18n/translation commits from Redmine validation #200
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: app
Are you sure you want to change the base?
Conversation
ekohl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the whole config is a bit excessive. There should only be a single pattern and I'd be inclined to hardcode it. Then you can use re.compile and reuse the compiled pattern. You can also simplify the code a lot by making it a simple is_exempt method on the Commit class. That keeps rest of the logic easier as well.
prprocessor/__main__.py
Outdated
|
|
||
| def is_commit_exempt(commit: Commit, exempt_patterns: list) -> bool: | ||
| """Check if commit subject matches any exemption pattern.""" | ||
| import re |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be a top level import?
prprocessor/__main__.py
Outdated
| for pattern in exempt_patterns: | ||
| if re.search(pattern, commit.subject, re.IGNORECASE): | ||
| return True | ||
| return False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a bit of code golfing
| for pattern in exempt_patterns: | |
| if re.search(pattern, commit.subject, re.IGNORECASE): | |
| return True | |
| return False | |
| return any(re.search(pattern, commit.subject, re.IGNORECASE) for pattern in exempt_patterns) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking about that too.
prprocessor/__main__.py
Outdated
| issue_ids.update(commit.refs) | ||
| if config.required and not commit.fixes and not commit.refs: | ||
| invalid_commits.append(commit) | ||
| if not is_commit_exempt(commit, config.exempt_patterns): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any particular reason you didn't add this to the existing if statement? Would the line become too long?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we had some specific (I do not exactly remember which one) flake ruleset enabled here, it would point out the very same thing. Will fix it.
prprocessor/config/repos.yaml
Outdated
| - "^i18n - " | ||
| - "^Localization - " | ||
| - "^Update translations" | ||
| - "^Automatic locale update" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like the many variations for this. In our code base we only have 2 and let's keep it that way
| - "^i18n - " | |
| - "^Localization - " | |
| - "^Update translations" | |
| - "^Automatic locale update" | |
| - "^i18n - (extracting new, )?pulling from tx$" |
Sources:
- https://github.com/theforeman/foreman/blob/ee1afb939ebfea220177b5066c4bb9744721cdfe/locale/Makefile#L62
- https://github.com/theforeman/foreman_plugin_template/blob/f92b585db9ddd4d8fc9c2bc3209162962157a67c/locale/Makefile#L70
- https://github.com/theforeman/hammer-cli/blob/737ec089d22d01aa537764dc9b6be0f1b8718a64/lib/hammer_cli/i18n/find_task.rb#L92
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, sort of wanted to have a discussion about this - if the i18n one is the only one we want to have in at this moment.
I'll leave only that one there then.
656d87a to
b338814
Compare
|
I've tried to address all your comments. Could you please re-review @ekohl? |
ekohl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I think this is a lot simpler to understand (and thus maintain).
prprocessor/__main__.py
Outdated
| re.IGNORECASE, | ||
| ) | ||
| COMMIT_ISSUES_REGEX = re.compile(r'#(\d+)') | ||
| EXEMPT_COMMIT_REGEX = re.compile(r'^i18n\s*-', re.IGNORECASE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can be more specific here to keep them standardized. Since they're typically generated by scripts that should be fine.
| EXEMPT_COMMIT_REGEX = re.compile(r'^i18n\s*-', re.IGNORECASE) | |
| EXEMPT_COMMIT_REGEX = re.compile(r'^i18n - (extracting new, )?pulling from tx$', re.IGNORECASE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed.
…dmine validation Add simple i18n commit exemption to allow translation commits to bypass Redmine validation requirements while maintaining validation for regular commits. Implementation: - Add hardcoded regex pattern for i18n commits (^i18n\s*- case insensitive) - Use compiled regex for performance - Add is_exempt() method to Commit class for clean encapsulation - Integrate exemption check into existing validation logic This allows commits like "i18n - Update Japanese translations" to pass validation without requiring Redmine issue references, addressing the workflow issue discussed in theforeman/theforeman-rel-eng#519. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
b338814 to
17148eb
Compare
Summary
Background
Resolves the issue discussed in theforeman/theforeman-rel-eng#519 (comment) where translation/internationalization commits often don't have specific Redmine issues associated with them, causing legitimate commits to fail validation.
Changes
exempt_patternsfield to Config dataclassis_commit_exempt()function with regex pattern matchingget_issues_from_pr()to check exemptions before marking commits invalidGlobal Patterns Added
"^i18n - "- Matches "i18n - Update Japanese translations""^Localization - "- Matches "Localization - Fix German strings""^Update translations"- Matches "Update translations for 3.5 release""^Automatic locale update"- Matches "Automatic locale update from Crowdin"Configuration Examples
Test plan
🤖 Generated with Claude Code