В письме от понедельник, 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
signature.asc
Description: This is a digitally signed message part.
