sherlock icon indicating copy to clipboard operation
sherlock copied to clipboard

Something for the 0.2 version: Some interface inconsistencies

Open kwisatz opened this issue 12 years ago • 4 comments

We've begun using facets and sorting in our application recently and it so happens that both use a different interface:

        $request->facets(
            Sherlock::facetBuilder()->Terms()->facetname('tags')->fields('tags.komma'),
            Sherlock::facetBuilder()->Terms()->facetname('types')->fields('type.keyword')
        );

        // Sort
        $request->sort(
            array(
                Sherlock::sortBuilder()->Field()->name('name.keyword'),
                Sherlock::sortBuilder()->Field()->name('_score')
            )
        );

sort() expects the parameters to be passed in an array, whereas facets() uses func_get_args() an expects one or more objects.

kwisatz avatar Jun 20 '13 15:06 kwisatz

Yeah, that is bad. Thanks for pointing it out, I'll make a note to clean it up. The convention I'm following for 0.2 is:

//Singular.  Repeated calls to `singular` add to the underlying value
$object->singular($value);
$object->singular($value);

//Plural with an array
$values = array($value, $value, $value);
$object->plural($values);

And maybe:

$object->plural($value, $value, $value);

I'm undecided on func_get_args() method calling at this point. I love the syntax, but it can introduce some strange edge cases when combined with the ability to pass in a single argument that is an array. The problem is trying to determine if it is a single array arg OR a single argument that happens to be an array.

It also makes IDE hinting difficult/impossible in some circumstances.

polyfractal avatar Jun 20 '13 16:06 polyfractal

in my fork, i've patched the facets method to work both ways...

// existing (function args syntax)
$request->facets(
    Sherlock::facetBuilder()->Terms()->facetname('tags')->fields('tags.komma'),
    Sherlock::facetBuilder()->Terms()->facetname('types')->fields('type.keyword')
);

and

// added (more consistent) array syntax
$request->facets(
    array(
        Sherlock::facetBuilder()->Terms()->facetname('tags')->fields('tags.komma'),
        Sherlock::facetBuilder()->Terms()->facetname('types')->fields('type.keyword')
    )
);

i would, personally, love to see this included in a new 0.1 release since we are using this in production and would like to point composer back to a tagged release (instead of my forked master)

jheys avatar Jul 03 '13 15:07 jheys

Sorry for the delay on replying to this, been travelling a lot. If you open up a pull request, I'd love to get this merged into 0.1.

You are a brave soul to be using Sherlock in production. Gonna give me nightmares now :P

polyfractal avatar Jul 10 '13 17:07 polyfractal

haha. i like to live dangerously!

jheys avatar Jul 10 '13 17:07 jheys