Обсуждение: Fixing typos in tests of partition_info.sql

Hi Amit,
(CC: -hackers)

I was just going through some of the tests, when I noticed that the
tests of partition_info.sql have two typos and that the last set of
tests is imprecise about the expected behavior of the functions.

Do you think that something like the attached is an improvement?

Thanks,
--
Michael

On Mon, Dec 17, 2018 at 03:40:28PM +0900, Michael Paquier wrote:
> I was just going through some of the tests, when I noticed that the
> tests of partition_info.sql have two typos and that the last set of
> tests is imprecise about the expected behavior of the functions.
>
> Do you think that something like the attached is an improvement?

Meuh.  Patch forgotten.
--
Michael

Hi,

On 2018/12/17 15:40, Michael Paquier wrote:
> Hi Amit,
> (CC: -hackers)
> 
> I was just going through some of the tests, when I noticed that the
> tests of partition_info.sql have two typos and that the last set of
> tests is imprecise about the expected behavior of the functions.
> 
> Do you think that something like the attached is an improvement?

You forgot to attach something. :)

Thanks,
Amit

On 2018/12/17 15:52, Michael Paquier wrote:
> On Mon, Dec 17, 2018 at 03:40:28PM +0900, Michael Paquier wrote:
>> I was just going through some of the tests, when I noticed that the
>> tests of partition_info.sql have two typos and that the last set of
>> tests is imprecise about the expected behavior of the functions.
>>
>> Do you think that something like the attached is an improvement?
> 
> Meuh.  Patch forgotten.

Thanks.

--- A table not part of a partition tree works is the only member listed.
+-- A table not part of a partition tree is the only member listed.

How about:

-- Table that is not part of any partition tree is the only member listed

--- Views and materialized viewS cannot be part of a partition tree.
+-- Views and materialized views are not part of a partition tree,
+-- causing the functions to return NULL.

How bouat:

-- Function returns NULL for relation types that cannot be part of a
-- partition tree; for example, views, materialized views, etc.

Thanks,
Amit

On Mon, Dec 17, 2018 at 04:14:07PM +0900, Amit Langote wrote:
> --- A table not part of a partition tree works is the only member listed.
> +-- A table not part of a partition tree is the only member listed.
>
> How about:
>
> -- Table that is not part of any partition tree is the only member listed
>
> --- Views and materialized viewS cannot be part of a partition tree.
> +-- Views and materialized views are not part of a partition tree,
> +-- causing the functions to return NULL.
>
> How about:
>
> -- Functions returns NULL for relation types that cannot be part of a
> -- partition tree; for example, views, materialized views, etc.

Your two suggestions look fine to me, so let's just reuse your wording.
i would just use the plural for the last comment with "Functions return"
as pg_partition_tree gets called multiple times, and later on
pg_partition_root will likely be added.

Perhaps there are other suggestions?
--
Michael

On 2018/12/17 16:38, Michael Paquier wrote:
> On Mon, Dec 17, 2018 at 04:14:07PM +0900, Amit Langote wrote:
>> --- A table not part of a partition tree works is the only member listed.
>> +-- A table not part of a partition tree is the only member listed.
>>
>> How about:
>>
>> -- Table that is not part of any partition tree is the only member listed
>>
>> --- Views and materialized viewS cannot be part of a partition tree.
>> +-- Views and materialized views are not part of a partition tree,
>> +-- causing the functions to return NULL.
>>
>> How about:
>>
>> -- Functions returns NULL for relation types that cannot be part of a
>> -- partition tree; for example, views, materialized views, etc.
> 
> Your two suggestions look fine to me, so let's just reuse your wording.
> i would just use the plural for the last comment with "Functions return"
> as pg_partition_tree gets called multiple times, and later on
> pg_partition_root will likely be added.

Okay, let's use "Functions" then, although I wonder if you shouldn't tweak
it later when you commit the pg_partition_root patch?

Thanks,
Amit

On Mon, Dec 17, 2018 at 04:41:01PM +0900, Amit Langote wrote:
> Okay, let's use "Functions" then, although I wonder if you shouldn't tweak
> it later when you commit the pg_partition_root patch?

There are already two calls to pg_partition_tree for each one of the two
relkinds tested.
--
Michael

On 2018/12/17 17:25, Michael Paquier wrote:
> On Mon, Dec 17, 2018 at 04:41:01PM +0900, Amit Langote wrote:
>> Okay, let's use "Functions" then, although I wonder if you shouldn't tweak
>> it later when you commit the pg_partition_root patch?
> 
> There are already two calls to pg_partition_tree for each one of the two
> relkinds tested.

You're saying that we should use plural "functions" because there of 2
*instances* of calling the function pg_partition_tree in the queries that
follow the comment, but I think that would be misleading.  I think the
plural would make sense if we're talking about two different functions,
but I may be wrong.

Thanks,
Amit

On Mon, Dec 17, 2018 at 05:56:08PM +0900, Amit Langote wrote:
> You're saying that we should use plural "functions" because there of 2
> *instances* of calling the function pg_partition_tree in the queries that
> follow the comment, but I think that would be misleading.  I think the
> plural would make sense if we're talking about two different functions,
> but I may be wrong.

Or this could just use "Function calls"?  My argument is just to not
forget about updating this comment later on and minimize future noise
diffs.
--
Michael

On 2018/12/17 18:10, Michael Paquier wrote:
> On Mon, Dec 17, 2018 at 05:56:08PM +0900, Amit Langote wrote:
>> You're saying that we should use plural "functions" because there of 2
>> *instances* of calling the function pg_partition_tree in the queries that
>> follow the comment, but I think that would be misleading.  I think the
>> plural would make sense if we're talking about two different functions,
>> but I may be wrong.
> 
> Or this could just use "Function calls"?

As far as the information content of this comment is concerned, I think
it'd be more useful to word this comment such that it is applicable to
different functions than to word it such that it is applicable to
different queries.  More opinions would be nice.

> My argument is just to not
> forget about updating this comment later on and minimize future noise
> diffs.

Okay, how about:

-- Various partitioning-related functions return NULL if passed relations
-- of types that cannot be part of a partition tree; for example, views,
-- materialized views, etc.

Thanks,
Amit

On Mon, Dec 17, 2018 at 06:35:03PM +0900, Amit Langote wrote:
> As far as the information content of this comment is concerned, I think
> it'd be more useful to word this comment such that it is applicable to
> different functions than to word it such that it is applicable to
> different queries.  More opinions would be nice.

This argument is sensible.

> Okay, how about:
>
> -- Various partitioning-related functions return NULL if passed relations
> -- of types that cannot be part of a partition tree; for example, views,
> -- materialized views, etc.

Okay, this suggestion sounds fine to me.  Thanks!
--
Michael

On Mon, Dec 17, 2018 at 07:49:42PM +0900, Michael Paquier wrote:
> Okay, this suggestion sounds fine to me.  Thanks!

And committed with all your suggestions included.  Thanks for the
discussion.
--
Michael

On 2018/12/18 10:57, Michael Paquier wrote:
> On Mon, Dec 17, 2018 at 07:49:42PM +0900, Michael Paquier wrote:
>> Okay, this suggestion sounds fine to me.  Thanks!
> 
> And committed with all your suggestions included.  Thanks for the
> discussion.

Thank you!

Regards,
Amit