В письме от понедельник, 24 июня 2024 г. 23:51:56 MSK пользователь Nikolay
Shaplov написал:
So, I continue reading the patch.
I see there is `entries` variable in the code, that is the list of
`RestrictInfo` objects and `entry` that is `OrClauseGroup` object.
This naming is quite misguiding (at least for me).
`entries` variable name can be used, as we deal only with RestrictInfo
entries here. It is kind of "generic" type. Though naming it
`restric_info_entry` might make te code more readable.
But when we come to an `entry` variable, it is very specific entry, it should
be `OrClauseGroup` entry, not just any entry. So I would suggest to name this
variable `or_clause_group_entry`, or even `or_clause_group` , so when we meet
this variable in the middle of the code, we can get the idea what we are
dealing with, without scrolling code up.
Variable naming is very important thing. It can drastically improve (or ruin)
code readability.
========
Also I see some changes in the tests int this patch. There are should be tests
that check that this new feature works well. And there are test whose behavior
have been just accidentally affected.
I whould suggest to split these tests into two patches, as they should be
reviewed in different ways. Functionality tests should be thoroughly checked
that all stuff we added is properly tested, and affected tests should be checked
that nothing important is not broken. It would be more easy to check if these
are two different patches.
I would also suggest to add to the commit message of affected tests changes
some explanation why this changes does not really breaks anything. This will
do the checking more simple.
To be continued.
--
Nikolay Shaplov aka Nataraj
Fuzzing Engineer at Postgres Professional
Matrix IM: @dhyan:nataraj.su
