Jakub Narebski [off-list ref] writes:
quoted
The lack of any real use of @fill_only in this patch also makes it hard to
judge if the new API gives a useful semantics. I would, without looking
at the real usage in 2/5 patch, naïvely expect that such a lazy filling
scheme would say "I am going to use A, B and C; I want to know if any of
them is missing, because I need values for all of them and I am going to
call a helper function to fill them if any of them is missing. Having A
and B is not enough for the purpose of this query, because I still need to
know C and I would call the helper function that computes all of them in
such a case. Even though it might be wasteful to recompute A and B,
computing all three at once is the only helper function available to me".
So for a person who does not have access to the real usage of the new API,
being able to give only a single $key *appears* make no sense at all, and
also the meaning of the @fill_only parameter is unclear, especially the
part that checks if that single $key appears in @fill_only.
...
information that is not already present. If @fill_only is nonempty, it
fills only selected information, again only if it is not already present.
@fill_only empty means no restrictions... which probably is not very obvious,
but is documented.
project_info_needs_filling() returns true if $key is not filled and is
interesting.
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"?
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.
On Fri, 10 Feb 2012, Junio C Hamano wrote:
Jakub Narebski [off-list ref] writes:
quoted
quoted
The lack of any real use of @fill_only in this patch also makes it hard to
judge if the new API gives a useful semantics. I would, without looking
at the real usage in 2/5 patch, naïvely expect that such a lazy filling
scheme would say "I am going to use A, B and C; I want to know if any of
them is missing, because I need values for all of them and I am going to
call a helper function to fill them if any of them is missing. Having A
and B is not enough for the purpose of this query, because I still need to
know C and I would call the helper function that computes all of them in
such a case. Even though it might be wasteful to recompute A and B,
computing all three at once is the only helper function available to me".
So for a person who does not have access to the real usage of the new API,
being able to give only a single $key *appears* make no sense at all, and
also the meaning of the @fill_only parameter is unclear, especially the
part that checks if that single $key appears in @fill_only.
...
information that is not already present. If @fill_only is nonempty, it
fills only selected information, again only if it is not already present.
@fill_only empty means no restrictions... which probably is not very obvious,
but is documented.
project_info_needs_filling() returns true if $key is not filled and is
interesting.
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;
}
which would translate to
if (project_info_needs_filled($pr, 'age') ||
project_info_needs_filled($pr, 'age_string') {
my (@activity) = git_get_last_activity($pr->{'path'});
unless (@activity) {
next PROJECT;
}
($pr->{'age'}, $pr->{'age_string'}) = @activity;
}
and then with @fill_only
if (project_info_needs_filled($pr, 'age', @fill_only) ||
project_info_needs_filled($pr, 'age_string', @fill_only) {
my (@activity) = git_get_last_activity($pr->{'path'});
unless (@activity) {
next PROJECT;
}
($pr->{'age'}, $pr->{'age_string'}) = @activity;
}
The same should be done for 'descr_long' and 'descr' which are also
always filled together.
--
Jakub Narebski
Poland