-
Notifications
You must be signed in to change notification settings - Fork 28
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
Implement the ~
merge operation
#234
Conversation
I'll add tests and docs for |
@LecrisUT can you resolve comments which you are happy about pls? |
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.
LGTM
Issue is that I can't. I can only resolve comments on PRs that I have opened. I'll give it one more look again tomorrow, but I think all of the issues are resolved. I think the only part is about
(actually maybe should still keep at least one case with |
Yeah. This limitation from github is really strange:/ I have no idea that why anyone who can comment is not suddenly allowed to resolve conversation they started. |
|
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.
Sorry for the late review, just a few minor touches and it's all 👍. You can resolve all of my previous discussions.
I like the functionality, but this might be too much regex magic from my PoV. It might get messy with the escapes, etc. |
That was an initial concern for me as well, but check the implementation of
Problem would be with regex groups (named/unnamed). I think that's an important feature to support for this. |
@LecrisUT No idea what what those are. Any chance you could enlighten me by pointing to how are they being used here, pretty please? |
An example is worth a hundred words :P. See the |
Thanks. I still don't get why it's needed, but guessing it is important when you want the users to be able to use regex. I was thinking more about allowing just some "simple" substitutions. :) |
Fixed a few typos and proposing some minor adjustments in 3a8ee40. |
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.
Just a few minor suggestions.
Does someone explain how can fedora-40 and c9s fail on |
Could be a race condition. Packit uses the merge commit and not the source commit iirc. But the srpm should be the same for all builds so not quite sure |
@thrix ? The passing job https://artifacts.dev.testing-farm.io/156d927c-3f15-47f6-8729-adcd97d0ef5f/#artifacts-/plans/features has
but failing one https://artifacts.dev.testing-farm.io/9db4c21f-82b9-4f8d-bceb-806577dec328/#artifacts-/plans/features has
Shouldn't be it the same? |
Ah, I guess you are right. Job ran 4h ago and merge (introducing additional_rules) happened also around that time. |
@psss I used 'repl' because |
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 for all the adjustments!
Co-authored-by: Cristian Le <[email protected]> Co-authored-by: Petr Šplíchal <[email protected]>
WDYT?
Todo: