Re: [libvirt PATCH v2 5/7] Add virNetworkObj Get and Set Methods for Metadata
My previous misunderstanding was exactly as you mentioned, to group commits on a file to
file basis.
Now I have understood what a semantic change should be like, with your provided example.
You are not asking for much, your corrections help me a lot in my learning journey and I
am very grateful for them.
Please feel free to point out any more such things in the future.
Shiva
On 25 August 2023 4:09:09 pm IST, "Michal Prívozník" <mprivozn(a)redhat.com>
wrote:
Again, way too much changes, disperse in semantics for one patch.
You've
introduced virNetworkObjUpdateModificationImpact(). Perfect! But it
should have been one patch. Then you eliminate redundant call to
virNetworkObjSetDefTransient()? Splendid, but again - it's a different
change and has nothing to do with virNetworkObjSetDefTransient(). You
implement new APIs? Sweet, but what do they have to do with the
redundant call?
Splitting patches per directory is not the same as "one semantic change
per patch". Sometimes it is, e.g. in the series I've posted earlier today:
https://listman.redhat.com/archives/libvir-list/2023-August/241416.html
I know I might be asking too much, but try to put yourself into
reviewers shoes. Libvirt's code base is not exactly the smallest and
reviewing one change (and trying to think of all implications) is hard
enough already. If changes are intertwined into one patch then it's
needlessly harder.
Michal
Attachments:
- attachment.html (text/html — 456 bytes)