@dmnks requested changes on this pull request.
The effort that went into splitting the commits is very much appreciated. That
said, I feel like it's a bit too granular for a patchset that we could merge
into master. As a general rule, we want each commit to be self-contained, i.e.
to compile successfully & pass the test-suite. It's crucial for future
bisect-abiilty.
Are those intermediate steps really necessary in order to be recorded in the
git history? If not, would you be willing to squash (some of) them?
For instance, the static `PyTypeObject*` declarations are removed in the first
couple of commits and their usages are only fixed later. I think they should
all be in a single commit since they belong to the same logical change.
Some commits also miss the *why*, e.g. the one titled `python rpmfile: Use the
type allocation function`.
A couple of minor style issues I've noticed:
1. `Python module isolation: Remove statically stored types`: the subject line
isn't separated by an empty line (breaking one-line git log output)
2. `Python module isolation: Visit PyObject* members in tp_visit hooks`: extra
leading space in the subject
--
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/3808#pullrequestreview-2932570131
You are receiving this because you are subscribed to this thread.
Message ID: <rpm-software-management/rpm/pull/3808/review/[email protected]>
_______________________________________________
Rpm-maint mailing list
[email protected]
https://lists.rpm.org/mailman/listinfo/rpm-maint
