On Fri, 10 Feb 2012, Junio C Hamano wrote:
> Jakub Narebski <jnareb@xxxxxxxxx> writes:
>> On Fri, 10 Feb 2012, Junio C Hamano wrote:
>> ...
>>> That still does not answer the fundamental issues I had with the presented
>>> API: why does it take only a single $key (please re-read my "A, B and C"
>>> example), and what does that single $key intersecting with @fill_only have
>>> anything to do with "needs-filling"?
>>
>> project_info_needs_filling() in absence of @fill_only is just a thin
>> wrapper around "!defined $pr->{$key}", it checks for each key if it needs
>> to be filled.
>>
>> It is used like this
>>
>> if (project_info_needs_filled("A", "A, B, C")) {
>> fill A
>> }
>> if (project_info_needs_filled("B", "A, B, C")) {
>> fill B
>> }
>> ...
>>
>>> After all, that 'age' check actually wants to fill 'age' and 'age_string'
>>> in the project. [...]
Thanks for noticing that... though I think more important is that some
further command would mark only 'age_string' as needed, and
fill_project_list_info() wouldn't notice that it needs to run
git_get_last_activity() if it checks only 'age'.
>> It is not as much matter of API, as the use of checks in loop in
>> fill_project_list_info().
>>
>> What is now
>>
>> my (@activity) = git_get_last_activity($pr->{'path'});
>> unless (@activity) {
>> next PROJECT;
>> }
>> ($pr->{'age'}, $pr->{'age_string'}) = @activity;
>>
>> should be
>>
>> if (!defined $pr->{'age'} ||
>> !defined $pr->{'age_string'}) {
>> my (@activity) = git_get_last_activity($pr->{'path'});
>> unless (@activity) {
>> next PROJECT;
>> }
>> ($pr->{'age'}, $pr->{'age_string'}) = @activity;
>> }
>
> Huh? Compare that with what you wrote above "It is used like this". This
> is *NOT* using the API like that. The caller knows it wants both age and
> age-string, and if even one of them is missing, do the work to fill both.
>
> So why isn't the info-needs-filled API not checking _both_ with a single
> call? It is only because you designed the API to accept only a single $key
> instead of list of "here are what I care about".
Passing two lists as parameters is a bit harder and makes for more
complicated API.
So what you mean is that instead of proposed
if (project_info_needs_filled($pr, 'age', @fill_only) ||
project_info_needs_filled($pr, 'age_string', @fill_only) {
it should be
if (project_info_needs_filled($pr, 'age', 'age_string', \@fill_only) {
or
if (project_info_needs_filled($pr, ['age', 'age_string'], @fill_only) {
(with either "..., 'owner', ..." or "..., [ 'owner' ], ..." for single-key
filling), or
if (project_info_needs_filled($pr, ['age', 'age_string'], \@fill_only) {
Is it?
--
Jakub Narebski
Poland
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html