changelog: ignore entries with wrong day padding #508
Conversation
Summary of ChangesHello @TomasTomecek, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses an issue where malformed changelog entries with incorrect day-of-month padding could propagate incorrect formatting. It introduces robust validation for RPM changelog day padding and modifies the entry addition logic to ignore invalid existing entries, ensuring that newly added changelog entries adhere to correct formatting standards. This significantly improves the reliability of changelog processing within the system. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively addresses an issue with incorrect day padding in changelog entries by introducing a validation property has_valid_day_padding and using it to filter out malformed entries when determining padding for new entries. The changes are logical, and the addition of both unit and integration tests ensures the fix is robust. I have one suggestion to make the regular expression in the new validation logic more precise for better long-term maintainability.
| m = re.search( | ||
| rf""" | ||
| ({weekdays}) # weekday | ||
| [ ] | ||
| ({months}) # month | ||
| [ ] | ||
| (?P<wsp>[ ]*) # optional whitespace padding | ||
| (?P<day>(?P<zp>0)?[1-9]\d*) # day of month (1-31) | ||
| [ ] # space after day | ||
| """, | ||
| self.header, | ||
| re.VERBOSE, | ||
| ) |
There was a problem hiding this comment.
The regular expression for parsing the day of the month is a bit too loose. [1-9]\\d* can match numbers greater than 31 (e.g., 99). While the subsequent integer conversion handles this for the purpose of checking padding, it's better practice to use a more precise regex to be stricter with the input format and avoid potential issues with malformed changelog entries.\n\nAdditionally, this regex is very similar to those in day_of_month_padding and extended_timestamp. There's an opportunity to reduce code duplication by extracting a common regex pattern or a helper method for date parsing in a future refactoring.
| m = re.search( | |
| rf""" | |
| ({weekdays}) # weekday | |
| [ ] | |
| ({months}) # month | |
| [ ] | |
| (?P<wsp>[ ]*) # optional whitespace padding | |
| (?P<day>(?P<zp>0)?[1-9]\d*) # day of month (1-31) | |
| [ ] # space after day | |
| """, | |
| self.header, | |
| re.VERBOSE, | |
| ) | |
| m = re.search( | |
| rf""" | |
| ({weekdays}) # weekday | |
| [ ] | |
| ({months}) # month | |
| [ ] | |
| (?P<wsp>[ ]*) # optional whitespace padding | |
| (?P<day>(?P<zp>0)?[1-9]|[12]\\d|3[01]) # day of month (1-31) | |
| [ ] # space after day | |
| """, | |
| self.header, | |
| re.VERBOSE, | |
| ) |
There was a problem hiding this comment.
And even better would be to make use of existing day_of_month_padding() without reimplementing everything from scratch. The biggest question here is how to get the info about nonsensical whitespace padding in a Pythonic way from day_of_month_padding() alongside its current return value, the rest is extremely trivial to implement.
|
❌ pre-commit FAILURE in 2m 00s |
|
Build succeeded. ✔️ pre-commit SUCCESS in 1m 58s |
|
Will update this based on Nikola's feedback. (Funny that Gemini didn't complain about that :) |
|
❌ pre-commit FAILURE in 1m 57s |
specfile/changelog.py
Outdated
|
|
||
| # Reject invalid padding: double-digit days with whitespace | ||
| if day_num >= 10 and wsp: | ||
| return "" |
There was a problem hiding this comment.
This doesn't seem very smart. Why not rather fix nonsensical padding and return it corrected? day_of_month_padding should be renamed to sanitized_day_of_month_padding or similar in such case, to match the implementation, and nothing else has to change.
There was a problem hiding this comment.
great point, will do and will cover it by a test case
specfile/changelog.py
Outdated
| [ ] | ||
| (?P<wsp>[ ]*) # optional whitespace padding | ||
| ((?P<zp>0)?\d|[12]\d|3[01]) # possibly zero-padded day of month | ||
| (?P<day>(?P<zp>0)?[1-9]\d*) # day of month (1-31) |
There was a problem hiding this comment.
What's wrong with a stricter regex? Are we assuming months will have more than 31 days in the future?
There was a problem hiding this comment.
I'm honestly surprised as well, because I saw the agent prefer the stricter regex but disbanded it in the second iteration; naughty claude
specfile/changelog.py
Outdated
| (?P<wsp>[ ]*) # optional whitespace padding | ||
| ((?P<zp>0)?\d|[12]\d|3[01]) # possibly zero-padded day of month | ||
| (?P<day>(?P<zp>0)?[1-9]\d*) # day of month (1-31) | ||
| [ ] # space after day |
There was a problem hiding this comment.
This doesn't have to be here, does it?
|
/gemini review |
|
Build succeeded. ✔️ pre-commit SUCCESS in 1m 57s |
There was a problem hiding this comment.
Code Review
This pull request effectively resolves the issue of malformed changelog entries with incorrect day padding. The changes in specfile/changelog.py enhance the robustness of the day_of_month_padding property by refining the regex for day parsing and introducing explicit error handling for invalid day formats and incorrect space padding for double-digit days. The addition of a default value to the max() function in specfile/specfile.py prevents potential ValueError exceptions when processing empty changelogs, ensuring a sensible default padding. The accompanying unit and integration tests have been thoughtfully updated and expanded to cover these new behaviors and edge cases, providing strong validation for the implemented solution.
|
✔️ pre-commit SUCCESS in 1m 58s |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request effectively addresses an issue with parsing changelog entries that have incorrect padding for the day of the month. The approach of updating the regular expression for better day parsing and sanitizing the padding is sound. The accompanying unit and integration tests are well-written and thoroughly validate the fix. I have a couple of minor suggestions to enhance the readability of the new logic in specfile/changelog.py, but overall, this is a solid contribution.
specfile/changelog.py
Outdated
| [ ] | ||
| (?P<wsp>[ ]*) # optional whitespace padding | ||
| ((?P<zp>0)?\d|[12]\d|3[01]) # possibly zero-padded day of month | ||
| ((?P<zp>0)?(?P<day>[12]\d|3[01]|[1-9])) # possibly zero-padded day of month |
There was a problem hiding this comment.
The regex for capturing the day of the month contains an unnecessary outer pair of parentheses. Removing them would simplify the expression without changing its behavior, improving readability.
| ((?P<zp>0)?(?P<day>[12]\d|3[01]|[1-9])) # possibly zero-padded day of month | |
| (?P<zp>0)?(?P<day>[12]\d|3[01]|[1-9]) # possibly zero-padded day of month |
There was a problem hiding this comment.
actually I'll split it to two lines
specfile/changelog.py
Outdated
| return ("" if len(m.group("day")) == 2 else m.group("wsp")) + ( | ||
| m.group("zp") or "" | ||
| ) |
There was a problem hiding this comment.
This conditional expression is a bit dense. Refactoring it into separate variables for whitespace and zero padding would improve readability and make the logic easier to follow.
| return ("" if len(m.group("day")) == 2 else m.group("wsp")) + ( | |
| m.group("zp") or "" | |
| ) | |
| whitespace_padding = "" if len(m.group("day")) == 2 else m.group("wsp") | |
| zero_padding = m.group("zp") or "" | |
| return whitespace_padding + zero_padding |
There was a problem hiding this comment.
Nikola hated that, so sorry mate, no can't do
There was a problem hiding this comment.
I mean, it seems quite self-explanatory to me even without the variables, but since the compact version doesn't fit on a single line anyway, I'm fine with the suggestion.
Alternative would be something like this, but let's go with the variables:
return (
("" if len(m.group("day")) == 2 else m.group("wsp")) +
(m.group("zp") or "")
)
|
Build succeeded. ✔️ pre-commit SUCCESS in 1m 56s |
|
Build succeeded. ✔️ pre-commit SUCCESS in 1m 57s |
|
Sorry, I didn't notice the tests failing on EL8. That has to be fixed. |
|
My bad, the other green ticks convinced me all is good so I ignored the EL failures. |
|
Build succeeded. ✔️ pre-commit SUCCESS in 1m 55s |
I had to fix the regex to correctly account for double-digit days (only first digit was parsed before this change). This commit also ignores extra padding for double-digit days. Signed-off-by: Tomas Tomecek <ttomecek@redhat.com> Assisted-by: Claude Co-authored-by: Nikola Forró <nforro@redhat.com>
|
Build succeeded. ✔️ pre-commit SUCCESS in 1m 57s |
|
regate |
|
Build succeeded (gate pipeline). ✔️ pre-commit SUCCESS in 1m 58s |
centosinfra-prod-github-app
bot
merged commit d0da008
into
packit:main
2 participants
This first version of this PR was completely implemented by Claude Code and Ambient Code.
After Nikola's amazing review, I fixed the problems myself, especially tinkering with padding fixups and updating the regex that parses day of month to properly account for double-digit days (and don't stop at the first digit - thanks Claude for that one).
Fixes #216
RELEASE NOTES BEGIN
Changelog entries that have incorrect padding set for a day of a month are now fixed if possible or ignored.
RELEASE NOTES END