On 18/05/2021 19:42, Laszlo Ersek wrote:
On 05/18/21 09:51, Ni, Ray wrote:
Thanks for explaining why you don't think it's a good patch. I thought anytime
changing a code,
I should try to make it better, functional better, looks better.
My only point was that separate concerns should be implemented in
separate patches, or at least (if they are really difficult, or
overkill, to isolate) that they should be documented.
Please try to think with your reviewers' mindsets in mind, when
preparing a patch (commit message and code both). The question the patch
author has to ask themselves is not only "how do I implement this", but
also "how do I explain this to my reviewers".
(Apologies in advance for intruding.)
Ray: I think you may be neglecting one half of the problem.
When making a code change, there are (at least) two requirements:
1. The new version of the code must make sense. You are definitely
achieving this: as you say, "make it better, functional better, looks
better". This is good.
2. The *change* between the old and new versions of the code (i.e. the
patch and accompanying commit message) must also make sense. This is
the requirement that I think Laszlo would like you to meet.
It's not viable for anyone to meaningfully review code unless both of
these requirements are met.
Thanks,
Michael
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#75419): https://edk2.groups.io/g/devel/message/75419
Mute This Topic: https://groups.io/mt/82765279/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
