Jakub Narebski <jnareb@xxxxxxxxx> 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.
--
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