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. Even if some other codepath starts filling 'age' in the
>> project with a later change, the current callers of fill_project_list_info
>> expects _both_ to be filled. So "I know the current implementation fills
>> both at the same time, so checking 'age' alone is sufficient" is not an
>> answer that shows good taste in the API design.
>
> 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".
--
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