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

Touch up of IElasticClient XMLDoc #598

Merged
merged 5 commits into from
Apr 27, 2014
Merged

Touch up of IElasticClient XMLDoc #598

merged 5 commits into from
Apr 27, 2014

Conversation

andersosthus
Copy link
Contributor

  • Added API reference URL to ScrollAsync
  • Changed API reference URL for Suggest and SuggestAsync to point to Suggest doc
  • Added subsection to some API references

- Changed the _Fuzzy property in CompletionSuggestDescriptor to
IFuzzySuggestDescriptor
- Updated FuzzySuggestDescriptor to implement IFuzzySuggestDescriptor
- Added FuzzinessSuggestDescriptor implementing IFuzzySuggestDescriptor
- Added .Fuzziness method to CompletionSuggestDescriptor
- Added unit tests in CompletionSuggestTests

Issues:
- Both the Fuzzy and Fuzziness methods returns a
CompletionSuggestDescriptor<T>, so that means you can chain .Fuzzy and
.Fuzziness. If you chain them, only the last one applied will be used.
That might not be a problem, depending on what level of knowledge is
expected from the developers using NEST.
@Mpdreamz
Copy link
Member

Thanks for attempting to add fuziness support to the completion suggester @andersosthus !

Ping me when you feel its ready :) Some comments so far:

Im Ok with Fuzzy() and Fuzziness() overriding eachother.

I don't think:

.Fuzziness();

Should generate:

"fuzzy": {
   "fuzziness": "auto"
};

As per the defaults section mentioned here: http://www.elasticsearch.org/guide/en/elasticsearch/reference/current/search-suggesters-completion.html#fuzzy

Sticking with the defaults:

sticky: {}

would suffice.

Lastly can you sign the Elasticsearch CLA:

http://www.elasticsearch.org/contributor-agreement/

So i can pull your bits in when they are done? 👍

@andersosthus
Copy link
Contributor Author

Hi @Mpdreamz,

Thanks for the quick feedback :)

First of all, do you want me to split this in two, one for the original Pull request, and one for the Fuzziness stuff ? I was a bit tired when I worked on this yesterday, so both PRs ended up in one :)

My reasoning for the "fuzziness": "auto" was that it was the default. I didn't see the note at the bottom stating that "fuzzy": {} will use the default.

I've changed .Fuzziness(); to now generate "fuzzy": {}, but won't that instead then default to using the "edit_distance" and "transpositions" defaults instead?

I've signed the Elasticsearch CLA :)

@andersosthus
Copy link
Contributor Author

Or were you maybe thinking that .Fuzziness(); should generate

"fuzzy": {
  "fuzziness": {}
}

.Fuzziness() now generates:
```
"fuzzy": {
"fuzziness": {}
}
```

Updated tests to reflect this. Also cleaned up the expected strings in
some cases (and my auto format did sneak in some spaces to tabs
conversions, but it looks like that's the desired format, so I hope you
don't mind).

Also tried to add a page to the documentation about Suggest.
@andersosthus
Copy link
Contributor Author

@Mpdreamz Can you take a look at it now ?

Think this should be about right

@andersosthus
Copy link
Contributor Author

Looks like my docs attempt made a small mess. Let me clean it up.

Mpdreamz added a commit that referenced this pull request Apr 27, 2014
@Mpdreamz Mpdreamz merged commit 1d651dd into elastic:master Apr 27, 2014
@Mpdreamz
Copy link
Member

Code, Tests & Docs in a single PR ! Awesome work @andersosthus.

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

Successfully merging this pull request may close these issues.

2 participants