Skip to content

Conversation

@lukapor
Copy link
Contributor

@lukapor lukapor commented Jun 13, 2014

No description provided.

@Mpdreamz Mpdreamz added this to the NEST 1.0 RC milestone Jun 16, 2014
@gmarz
Copy link
Contributor

gmarz commented Jul 8, 2014

Hey @lukapor, thank you for this. Your PR inspired #748, which defines a SimilarityDescriptor and provides the ability to set similarities via the fluent API approach that all our descriptors take.

Regarding your change to SortFieldDescriptor, I believe ToggleSort is correct the way it is. It's toggling the sort order, not just setting it- so if the current order is Ascending, then it would be correct to change it to Descending.

Thanks again for your PR, sorry I couldn't merge it in!

@gmarz gmarz closed this Jul 8, 2014
@lukapor
Copy link
Contributor Author

lukapor commented Jul 8, 2014

Hey ok it is maybe confusing method name, but it isn't consistent with same function in SortScriptDescriptor.cs and SortFieldDescriptor.cs. Please check it.

@lukapor
Copy link
Contributor Author

lukapor commented Jul 8, 2014

And SortGeoDistanceDescriptor.cs file.

@gmarz
Copy link
Contributor

gmarz commented Jul 9, 2014

@lukapor You are totally right, thanks for pointing that out. ToggleSort is completely confusing. I just opened PR #764 which replaces ToggleSort() with Order() and takes the SortOrder enum instead of a bool. I think that's much clearer and just as effective- what do you think?

@lukapor
Copy link
Contributor Author

lukapor commented Jul 9, 2014

That is great. Thanks.

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.

3 participants