Jakub Narebski [off-list ref] writes:
Introduce project_info_needs_filling($pr, $key[, \%fill_only]), which
That's "project_info_needs_filling($pr, @key[, \%fill_only])", isn't it?
+# entry for given @keys needs filling if at least one of interesting keys
+# in list is not present in %$project_info; key is interesting if $fill_only
+# is not passed, or is empty (all keys are interesting in both of those cases),
+# or if key is in $fill_only hash
+#
+# USAGE:
+# * project_info_needs_filling($project_info, 'key', ...)
+# * project_info_needs_filling($project_info, 'key', ..., \%fill_only)
+# where %fill_only = map { $_ => 1 } @fill_only;
The reason this spells bad to me is that it gives the readers (and anybody
who may want to touch gitweb code) that the caller has _two_ ways to say
the same thing.
If a caller is interested in finding out $key in $project_info, it can
either (1) pass $key as one of the earlier parameters to the function and
not pass any \%fill_only, or (2) pass $key and pass \%fill_only but after
making sure \%fill_only also has $key defined.
And if the caller passes \%fill_only, it has to remember that it needs to
add $key to it; otherwise the earlier request "I want $key" is ignored by
the function.
Why is it a good thing to have such a convoluted API?
Is it that "fill_project_list_info" is called by multiple callers, and
they do not necessarily need to see all the fields that can be filled by
the function (namely, age, age_string, descr, descr_long, owner, ctags and
category)? If that is the case, I must say that the approach to put the
set filtering logic inside project_info_needs_filling function smells like
a bad API design.
In other words, wouldn't a callchain that uses a more natural set of API
functions be like this?
# the top-level caller that is interested only in these two fields
fill_project_list_info($projlist, 'age', 'owner');
# in fill_project_list_info()
my $projlist = shift;
my %wanted = map { $_ => 1 } @_;
foreach my $pr (@$projlist) {
if (project_info_needs_filling($pr,
filter_set(\%wanted, 'age', 'age_string'))) {
... get @activity ...
($pr->{'age'}, $pr->{'age_string'}) = @activity;
}
...
}
That way, "needs_filling" has to do one and only one thing well: it checks
if any of the fields it was asked is missing and answers. And a generic
set filtering helper that takes a filtering hashref and an array can be
used later outside of the "needs_filling" function.
sub project_info_needs_filling {
+ my $fill_only = ref($_[-1]) ? pop : undef;
my ($project_info, @keys) = @_;
# return List::MoreUtils::any { !exists $project_info->{$_} } @keys;
foreach my $key (@keys) {
- if (!exists $project_info->{$key}) {
+ if ((!$fill_only || !%$fill_only || $fill_only->{$key}) &&
+ !exists $project_info->{$key}) {
return 1;
}
}
return;
}
-
Shouldn't the previous patch updated not to add this blank line in the
first place?
# fills project list info (age, description, owner, category, forks)
Is this enumeration up-to-date? If we cannot keep it up-to-date, it may
make sense to show only a few as examples and add ellipses.
# for each project in the list, removing invalid projects from
-# returned list
+# returned list, or fill only specified info (removing invalid projects
+# only when filling 'age').
It is unclear what the added clause wants to say; especially, the link
between the mention of 'age' and 'only when' is unclear. Is asking of
'age' what makes a request a notable exception to remove invalid projects?
Or is asking to partially fill fields what triggers the removal of invalid
projects, and you mentioned 'age' as an example?
If it is the former (I am guessing it is), it would be much clearer to
say:
Invalid projects are removed from the returned list if and only if you
ask 'age' to be filled, because of such and such reasons.
On Mon, 20 Feb 2012, Junio C Hamano wrote:
Jakub Narebski [off-list ref] writes:
quoted
Introduce project_info_needs_filling($pr, $key[, \%fill_only]), which
That's "project_info_needs_filling($pr, @key[, \%fill_only])", isn't it?
Yes it is. I'm sorry, I have missed that this needs fixing from the
first version of patch series. Since then I have noticed that in few
cases we fill two fields at once, so we have to ask about two fields
at once as well.
quoted
+# entry for given @keys needs filling if at least one of interesting keys
+# in list is not present in %$project_info; key is interesting if $fill_only
+# is not passed, or is empty (all keys are interesting in both of those cases),
+# or if key is in $fill_only hash
+#
+# USAGE:
+# * project_info_needs_filling($project_info, 'key', ...)
+# * project_info_needs_filling($project_info, 'key', ..., \%fill_only)
+# where %fill_only = map { $_ => 1 } @fill_only;
The reason this spells bad to me is that it gives the readers (and anybody
who may want to touch gitweb code) that the caller has _two_ ways to say
the same thing.
If a caller is interested in finding out $key in $project_info, it can
either (1) pass $key as one of the earlier parameters to the function and
not pass any \%fill_only, or (2) pass $key and pass \%fill_only but after
making sure \%fill_only also has $key defined.
And if the caller passes \%fill_only, it has to remember that it needs to
add $key to it; otherwise the earlier request "I want $key" is ignored by
the function.
Yes, you are right. All of this is caused by fill_project_list_info()
trying to do *two* things, instead of doing one thing and doing it well,
as is Unix philosophy. It tried to check both that key is needed and
that key is wanted.
Why is it a good thing to have such a convoluted API?
Thanks for a sanity check.
Is it that "fill_project_list_info" is called by multiple callers, and
they do not necessarily need to see all the fields that can be filled by
the function (namely, age, age_string, descr, descr_long, owner, ctags and
category)? If that is the case, I must say that the approach to put the
set filtering logic inside project_info_needs_filling function smells like
a bad API design.
You are right.
In other words, wouldn't a callchain that uses a more natural set of API
functions be like this?
# the top-level caller that is interested only in these two fields
fill_project_list_info($projlist, 'age', 'owner');
# in fill_project_list_info()
my $projlist = shift;
my %wanted = map { $_ => 1 } @_;
foreach my $pr (@$projlist) {
if (project_info_needs_filling($pr,
filter_set(\%wanted, 'age', 'age_string'))) {
That or
if (project_info_needs_filling($pr, 'age', 'age_string') &&
project_info_is_wanted(\%wanted, 'age', 'age_string')) {
and in your case there is no duplication of field names.
filter_set() is simply intersection of two sets, but it treats empty
%wanted as a full set.
... get @activity ...
($pr->{'age'}, $pr->{'age_string'}) = @activity;
}
...
}
That way, "needs_filling" has to do one and only one thing well: it checks
if any of the fields it was asked is missing and answers. And a generic
set filtering helper that takes a filtering hashref and an array can be
used later outside of the "needs_filling" function.
Right.
quoted
sub project_info_needs_filling {
+ my $fill_only = ref($_[-1]) ? pop : undef;
my ($project_info, @keys) = @_;
# return List::MoreUtils::any { !exists $project_info->{$_} } @keys;
foreach my $key (@keys) {
- if (!exists $project_info->{$key}) {
+ if ((!$fill_only || !%$fill_only || $fill_only->{$key}) &&
+ !exists $project_info->{$key}) {
return 1;
}
}
return;
}
-
Shouldn't the previous patch updated not to add this blank line in the
first place?
O.K.
quoted
# fills project list info (age, description, owner, category, forks)
Is this enumeration up-to-date? If we cannot keep it up-to-date, it may
make sense to show only a few as examples and add ellipses.
O.K.
quoted
# for each project in the list, removing invalid projects from
-# returned list
+# returned list, or fill only specified info (removing invalid projects
+# only when filling 'age').
It is unclear what the added clause wants to say; especially, the link
between the mention of 'age' and 'only when' is unclear. Is asking of
'age' what makes a request a notable exception to remove invalid projects?
Or is asking to partially fill fields what triggers the removal of invalid
projects, and you mentioned 'age' as an example?
If it is the former (I am guessing it is), it would be much clearer to
say:
Invalid projects are removed from the returned list if and only if you
ask 'age' to be filled, because of such and such reasons.
O.K.
Sidenote: the reason is that to check that project is valid you really
have to run git command that requires repository in it[1]. Only when
filling 'age' and 'age_string' using git_get_last_activity() we always
run git command.
[1] Gitweb actually uses --git-dir=<PATH> parameter to git wrapper.
--
Jakub Narebski
Poland