Re: [PATCHv2 2/8] gitweb: Option for filling only specified info in fill_project_list_info

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 20, 2012, 06:58 UTC
Message-ID: <7vfwe6ng8u.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1329338332-30358-3-git-send-email-jnareb@gmail.com>

Jakub Narebski <jnareb@gmail.com> writes:

> Introduce project_info_needs_filling($pr, $key[, \%fill_only]), which

That's "project_info_needs_filling($pr, @key[, \%fill_only])", isn't it?

Show 9 quoted lines

> +# 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.

Show 16 quoted lines

>  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.