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

Use float? for suggester fractional properties #3661

Merged
merged 2 commits into from
Apr 12, 2019
Merged

Conversation

russcam
Copy link
Contributor

@russcam russcam commented Apr 11, 2019

This commit updates TermSuggester to use float? instead of decimal? for
properties that take a float value.

Add XML documentation to suggesters

This commit updates TermSuggester to use float? instead of decimal? for
properties that take a float value.

Add XML documentation to suggesters
Copy link
Member

@Mpdreamz Mpdreamz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this looks 99% LGTM, soo many great xml docs additions.

I left some small nagging comments about 1% of the new xmldocs additions.

public class CompletionField
{
/// <summary>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hate the hate on adding xmldocs but not fan of these style of SandCastle'esque summaries that feel generated. I rather have none instead.

Further more to continue my nitpick for small summaries can we have a single line

//// <summary> The summary </summary>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for small summaries can we have a single line

I can do this. We'll also need to tweak our formatting configuration which always seems to format on multiple lines

@@ -3,35 +3,61 @@

namespace Nest
{
/// <summary>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FuzzySugester is a bad name now that I am rereading it maybe SuggesterFuzziness?

Maybe link to https://www.elastic.co/guide/en/elasticsearch/reference/current/search-suggesters-completion.html#fuzzy ?

The comment itself as it is now adds little IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

++ to SuggesterFuzziness; it better fits what it is.

I share some of the sentiment of GhostDoc/Sandcastle autogenerated XML comments. Do you think we should aim for consistency in applying <summary>?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the naming convention of other types, ISuggestFuzziness might be more appropriate

@russcam
Copy link
Contributor Author

russcam commented Apr 12, 2019

I'm merging this in, to look to get an alpha release out

@russcam russcam merged commit 444856c into 7.x Apr 12, 2019
@Mpdreamz Mpdreamz deleted the fix/decimal-usage branch June 17, 2019 12:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants