-
Notifications
You must be signed in to change notification settings - Fork 20
Add author and timestamp properties to ChangelogEntry #482
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: main
Are you sure you want to change the base?
Conversation
These properties are required to convert Fedora to openSUSE style changelogs for packit
|
Build failed. ✔️ pre-commit SUCCESS in 1m 53s |
|
So, it looks like the tests only worked locally for me by accident, because I'm in the CEST timezone and I do have more timezone information available locally. To properly parse the timezone, we'd have to add another dependency. What's your opinion on this? Add a dependency or live with wrong timezones? |
nforro
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.
To properly parse the timezone, we'd have to add another dependency. What's your opinion on this? Add a dependency or live with wrong timezones?
That's one of the reasons this was never implemented. Wrong timezones are a no-go, if we are going to implement this, let's do it right.
| if not self.evr: | ||
| return author_and_evr | ||
|
|
||
| if (gt_ind := author_and_evr.rfind(">")) > 0: |
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.
This assumes the author substring always ends with a specifically formatted e-mail, but that doesn't have to be the case, even though Fedora Packaging Guidelines mandate so. specfile should be able to parse any format supported by RPM, which allows any string after the timestamp (ended by a newline) - and refers to it as name. What about adding a more generic name property and then deriving author and evr from it?
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 have refactored the code a bit, so that it no longer relies on the > character to be present. But I am not sure if we should try to parse such a changelog correctly anyway. Imagine the following entry:
* Mon May 22 2023 $FirstName $LastName
Our evr regex will recognize $LastName as the EVR in this case and given that we allow anything as the evr, there's no way to distinguish it from a last name. At least nothing immediate comes to my mind.
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.
That's true, but there are lots of spec files with changelog entries like this, for example:
https://src.fedoraproject.org/rpms/gdb/blob/rawhide/f/gdb.spec#_1516
https://src.fedoraproject.org/rpms/rpm-spec-language-server/blob/rawhide/f/rpm-spec-language-server.spec#_72
We could try to recognize these formats, and document that author/EVR parsing is best effort and doesn't have to be accurate:
* timestamp author - EVR* timestamp author [EVR]* timestamp author (EVR)* timestamp author
Perhaps adding another dependency could be avoided by matching the entries to RPM-parsed changelog - |
These properties are required to convert Fedora to openSUSE style changelogs for packit
TODO:
packit/packit.dev.Fixes
Related to
Merge before/after
RELEASE NOTES BEGIN
RELEASE NOTES END