Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Something for the 0.2 version: Some interface inconsistencies #64

Open
kwisatz opened this issue Jun 20, 2013 · 4 comments
Open

Something for the 0.2 version: Some interface inconsistencies #64

kwisatz opened this issue Jun 20, 2013 · 4 comments

Comments

@kwisatz
Copy link
Contributor

kwisatz commented Jun 20, 2013

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.

@polyfractal
Copy link
Owner

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.

@jheys
Copy link
Contributor

jheys commented Jul 3, 2013

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)

@polyfractal
Copy link
Owner

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

@jheys
Copy link
Contributor

jheys commented Jul 10, 2013

haha. i like to live dangerously!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants