-
Notifications
You must be signed in to change notification settings - Fork 414
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
dnf history
operations that work with comps correctly
#1689
Conversation
Hello @dmach! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2021-01-07 10:03:46 UTC |
This pull request introduces 4 alerts and fixes 3 when merging 5e414e1 into e5b25b8 - view on LGTM.com new alerts:
fixed alerts:
|
This pull request introduces 2 alerts and fixes 3 when merging 12f9677 into e5b25b8 - view on LGTM.com new alerts:
fixed alerts:
|
This pull request introduces 2 alerts and fixes 3 when merging 733bcc4 into e5b25b8 - view on LGTM.com new alerts:
fixed alerts:
|
dnf/transaction_sr.py
Outdated
@@ -220,25 +219,31 @@ def __init__( | |||
self._nevra_reason_cache = {} | |||
self._warnings = [] | |||
|
|||
def load_from_file(self, fn): |
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'm not sure whether it's a better API to have load_from_file()
and load_from_dict()
methods aor whether it would be better to have an init like __init__(self, base, data=None, filename=None, ...)
and then you'd specify one of [data
, filename
] when instantiating the class? I know there's no RAII in Python 🙂 but the class now requires you to call a load method and if you don't, you'll get a rather unspecific crash (though it's a programmer error).
This pull request introduces 4 alerts and fixes 3 when merging b7f4a62 into d8c71d2 - view on LGTM.com new alerts:
fixed alerts:
|
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 restructuring the commits! I still think the code needs to be adjusted for the new usage, particularly the error handling part. I'm not too proud of the errors/warnings code myself and this makes it even more cumbersome, I think it still deserves some work.
Congratulations! One of the builds has completed. 🍾 You can install the built RPMs by following these steps:
Please note that the RPMs should be used only in a testing environment. |
This pull request introduces 2 alerts and fixes 3 when merging 73575e5 into b73d813 - view on LGTM.com new alerts:
fixed alerts:
|
This pull request introduces 8 alerts and fixes 3 when merging fcdef43 into 11ce84b - view on LGTM.com new alerts:
fixed alerts:
|
dnf/transaction_sr.py
Outdated
): | ||
""" | ||
:param base: the dnf base | ||
:param fn: the filename to load the transaction from | ||
:param ignore_extras: whether to ignore extra package pulled into the transaction | ||
:param ignore_installed: whether to ignore installed versions of packages | ||
:param skip_unavailable: whether to skip transaction packages that aren't available | ||
:param history_mode: enable a more permissive behavior for `dnf history {undo|redo|rollback}` commands |
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 looks a bit like a rudimentary brace. The description of the new argument doesn't say anything about its effect, just refers to it being related to history. On this level the switch should be based on its semantics and not on it's usage. The logic around this is again already non-trivial and this muddies up the waters even more.
This pull request introduces 8 alerts and fixes 3 when merging 32720cc into 11ce84b - view on LGTM.com new alerts:
fixed alerts:
|
This pull request introduces 8 alerts and fixes 3 when merging dc4bedd into 11ce84b - view on LGTM.com new alerts:
fixed alerts:
|
dnf/transaction_sr.py
Outdated
@@ -187,14 +187,16 @@ class TransactionReplay(object): | |||
def __init__( | |||
self, | |||
base, | |||
fn, | |||
fn="", |
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 you please just name the argument filename
instead of fn
? It's a bit more important since it's now mutually exclusive with the data
and they both should be self-descriptive, not abbreviated. Otherwise looks good.
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. I hope it did not break anything and the tests will pass.
= changelog = msg: Support comps groups in history redo type: enhancement resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1657123 resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1809565 resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1809639
= changelog = msg: Support comps groups in history rollback type: enhancement resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1657123 resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1809565 resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1809639
= changelog = msg: Support comps groups in history undo type: enhancement resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1657123 resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1809565 resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1809639
When _ignore_installed == True, then an exception is raised anyway. When _ignore_installed == False, get the requested package to the system regardless the action.
This pull request introduces 8 alerts and fixes 3 when merging 49d8837 into 11ce84b - view on LGTM.com new alerts:
fixed alerts:
|
Tests: rpm-software-management/ci-dnf-stack#916