Skip to content
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 alternative transaction callback style #1365

Merged
merged 1 commit into from Oct 5, 2020

Conversation

pmatilai
Copy link
Member

Add API to get and set callback style, defaulting to the good ole
header first style and add a new style where a transaction element
is passed in place of the header, allowing a much saner interaction.

This is doubly so in Python, where the old style callback is a
braindamaged mess. In the new mode the "key" is not passed as separate
argument at all, it can be just as well retrieved via te.Key() for
the callbacks needing it.

@pmatilai
Copy link
Member Author

pmatilai commented Sep 21, 2020

To clarify, in the C API the only change when the new mode is enabled, is the first callback-argument changing from a header to rpmte, ie (rpmte, what, amount, total, key, userdata). Key becomes redundant in this mode and could be dropped, because you can trivially get it from the rpmte now, but that would make this much more complicated for little to no gain, so leaving it alone.

The Python callback is a very different animal. The old-style arguments are (what, amount, total, pkgObj, userdata), with pkgObj being either rpmteKey(), RPMTAG_NAME string or None. In this (draft) version, the arguments become: (rpmte, what, amount, total, userdata) in the new mode, ie almost the same as new mode C API. The question is whether we should make it the equivalent of the C API, ie with a "key" argument. It's redundant, so I'd kinda prefer to drop it from this new API, but then here's a chance to stop the forever old confusion due to Python and C callback being different...

Thoughts?

@jrohel
Copy link

jrohel commented Sep 22, 2020

@pmatilai
Python API:
From my point of view the new mode arguments (rpmte, what, amount, total, userdata) are much better than the old ones. And it is almost the same as in the new C API mode.
I prefer to drop "key" argument from this new Python API.

@pmatilai
Copy link
Member Author

@jrohel , thanks for the feedback.

Another option wrt Python callback could be just replacing the crazy pkgObj thing with rpmte, so the order and number of arguments would actually remain as it was, just that the weird pkgObj becomes actually meaningful, consistent and always present. Python being Python, this could be dynamically detected and handled compatibly with a callback that handles the old style too, but sometimes a clean break is better...

@pmatilai
Copy link
Member Author

Another thing to consider wrt the Python API is the way the new callback style is enabled. Here it's an added argument to ts.run() because that was the cheapest way to get it working, but testing for such a thing at runtime is not nice. Probably the better option would be just copying the C API, ie add a new ts.setCallbackStyle() method that persistently sets the style for a given ts.

@pmatilai
Copy link
Member Author

Updated for various fixes (mostly on Python side):

  • not all callbacks have a transaction element (so we must pass None there)
  • make callback style a persistent ts property so support is easier to test for and doesn't need to be set for every ts.run() call
  • add a brief testcase to demonstrate the python args (old and new)

…ments

Add API to get and set callback style, defaulting to the good ole
header first style and add a new style where a transaction element
is passed in place of the header, allowing a much saner interaction.

This is doubly so in Python, where the old style callback is a
braindamaged mess. In the new mode the "key" is not passed as separate
argument at all, it can be just as well retrieved via te.Key() for
the callbacks needing it.
@pmatilai
Copy link
Member Author

pmatilai commented Oct 1, 2020

Rebased to solve the conflict.

@pmatilai
Copy link
Member Author

pmatilai commented Oct 5, 2020

Since there's no further feedback... (and there's still plenty of time to fine-tune details before next rpm release)

@pmatilai pmatilai merged commit ef3ea9c into rpm-software-management:master Oct 5, 2020
@pmatilai pmatilai deleted the cbargs-pr branch October 19, 2020 08:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API API related python Python bindings RFE
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants