Skip to content

Conversation

gmoskovicz
Copy link
Contributor

Support for #1433

@gmoskovicz gmoskovicz added the WIP label Jun 8, 2015
var page = 0;
Func<SearchDescriptor<T>, SearchDescriptor<T>> searchDescriptor = s => s.Index(fromIndex);

if (typeof(T).Name.Equals(typeof(object).Name))
Copy link
Member

Choose a reason for hiding this comment

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

I think this fix should be moved to the non generic overload, this might still cause confusion when someone actually calls Reindex<objecT>()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Mpdreamz i did not understand this one :). What should we do if it comes as generic object? I thought the idea was to search all types?

Copy link
Member

Choose a reason for hiding this comment

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

Here: https://github.com/elastic/elasticsearch-net/blob/1433-support-generic-reindex/src/Nest/ElasticClient-Reindex.cs#L19 we added a non generic overload to the client. However we can not distinguish here between someone calling the generic or non generic overload. If someone now calls client.Reindex<object>() it will reindex on everything where as only client.Reindex() should.

@@ -14,5 +14,11 @@ public IObservable<IReindexResponse<T>> Reindex<T>(Func<ReindexDescriptor<T>, Re
var observable = new ReindexObservable<T>(this, _connectionSettings, reindexDescriptor);
return observable;
}

/// <inheritdoc />
public IObservable<IReindexResponse<IDocument>> Reindex(Func<ReindexDescriptor<object>, ReindexDescriptor<object>> reindexSelector)
Copy link
Member

Choose a reason for hiding this comment

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

To be truly lazy IDocument this signature needs to change to Reindex(Func<ReindexDescriptor<IDocument>, ReindexDescriptor<IDocument>> reindexSelector)

@gmoskovicz
Copy link
Contributor Author

@Mpdreamz is this what you expected? I dont know but something is telling me that i've missed something :)

Mpdreamz added a commit that referenced this pull request Jul 17, 2015
@Mpdreamz
Copy link
Member

err meant #1404 there :)

@Mpdreamz
Copy link
Member

No this looks good to me! 👍

Mpdreamz added a commit that referenced this pull request Jul 17, 2015
@Mpdreamz Mpdreamz merged commit 43b58bc into develop Jul 17, 2015
@Mpdreamz Mpdreamz deleted the 1433-support-generic-reindex branch July 17, 2015 13:57
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