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

Try to streamline the update api's #795

Merged
merged 3 commits into from Jul 16, 2014

Conversation

Projects
None yet
4 participants
@Mpdreamz
Member

Mpdreamz commented Jul 16, 2014

And come up with an API thats verbose in its intend leaving no room for confusion.

@Mpdreamz Mpdreamz referenced this pull request Jul 16, 2014

Closed

Update<T,K> #732

@Mpdreamz

This comment has been minimized.

Show comment
Hide comment
@Mpdreamz

Mpdreamz Jul 16, 2014

Member

Ok huge breaking change post RC1 but I feel its worth swollowing our pride to come up with a better public API

.Object(T doc) => .Id(T doc)
.PartialUpdate(T doc) => .PartialDocument(T doc)
.DocAsUpsert() => PartialDocumentAsUpsert()

in the update and bulk update api the .Id(T id) also has an .Id(T id, bool useAsUpsert) overload.

Furthermore the generic type names for UpdateDescriptor and BulkUpdateDescriptor have also been streamlined

See:

https://github.com/elasticsearch/elasticsearch-net/blob/8f5e5dd012bf7f1780cccb52c43956cdf47dbb37/src/Tests/Nest.Tests.Unit/Core/Update/UpdateAPIConsistencyTests.cs#L35

For some actual examples.

Member

Mpdreamz commented Jul 16, 2014

Ok huge breaking change post RC1 but I feel its worth swollowing our pride to come up with a better public API

.Object(T doc) => .Id(T doc)
.PartialUpdate(T doc) => .PartialDocument(T doc)
.DocAsUpsert() => PartialDocumentAsUpsert()

in the update and bulk update api the .Id(T id) also has an .Id(T id, bool useAsUpsert) overload.

Furthermore the generic type names for UpdateDescriptor and BulkUpdateDescriptor have also been streamlined

See:

https://github.com/elasticsearch/elasticsearch-net/blob/8f5e5dd012bf7f1780cccb52c43956cdf47dbb37/src/Tests/Nest.Tests.Unit/Core/Update/UpdateAPIConsistencyTests.cs#L35

For some actual examples.

@gmarz

This comment has been minimized.

Show comment
Hide comment
@gmarz

gmarz Jul 16, 2014

Member

@Mpdreamz I think this is much better.

My only suggestion would be to maybe rename Id(T doc) to IdFrom(T doc). I think the latter is a bit more clear on what the intentions of the method are (inferring the Id from the given document). What do you think?

Member

gmarz commented Jul 16, 2014

@Mpdreamz I think this is much better.

My only suggestion would be to maybe rename Id(T doc) to IdFrom(T doc). I think the latter is a bit more clear on what the intentions of the method are (inferring the Id from the given document). What do you think?

@nariman-haghighi

This comment has been minimized.

Show comment
Hide comment
@nariman-haghighi

nariman-haghighi Jul 16, 2014

For consistency and alignment with ES itself, I would prefer to see Just .Doc() and .DocAsUpsert(), with an error thrown if the specified doc didn't provide an ID along with the partial update fields. Without knowing any NEST specifics, that's the usage I would have inferred from glancing at ES docs.

nariman-haghighi commented Jul 16, 2014

For consistency and alignment with ES itself, I would prefer to see Just .Doc() and .DocAsUpsert(), with an error thrown if the specified doc didn't provide an ID along with the partial update fields. Without knowing any NEST specifics, that's the usage I would have inferred from glancing at ES docs.

@Mpdreamz

This comment has been minimized.

Show comment
Hide comment
@Mpdreamz

Mpdreamz Jul 16, 2014

Member

Thats the confusing part. The doc does not have to be complete and does not have to provide an Id.

.Update<MyDoc, MyUpdate>(u=>u
      .IdFrom(myDoc)
      .Script("")
)

Is also valid update statement, we currently already throw if we can not get an id and the object initializer syntax forces you to specify an id on the constructor.

But your point is valid, now we no longer have .Object and .Document should we name .PartialDocument() to .Doc() to match closer to the elasticsearch DSL (which is one of NESTs design goals) or do we expand .Doc() to its more verbose meaning .PartialDocument() to align better with C# guidelines.

Thoughts?

@gmarz i'm +1 on IdFrom()

Member

Mpdreamz commented Jul 16, 2014

Thats the confusing part. The doc does not have to be complete and does not have to provide an Id.

.Update<MyDoc, MyUpdate>(u=>u
      .IdFrom(myDoc)
      .Script("")
)

Is also valid update statement, we currently already throw if we can not get an id and the object initializer syntax forces you to specify an id on the constructor.

But your point is valid, now we no longer have .Object and .Document should we name .PartialDocument() to .Doc() to match closer to the elasticsearch DSL (which is one of NESTs design goals) or do we expand .Doc() to its more verbose meaning .PartialDocument() to align better with C# guidelines.

Thoughts?

@gmarz i'm +1 on IdFrom()

@nariman-haghighi

This comment has been minimized.

Show comment
Hide comment
@nariman-haghighi

nariman-haghighi Jul 16, 2014

That makes sense. I'd go with .IdFrom() .Doc() and .DocAsUpsert()

nariman-haghighi commented Jul 16, 2014

That makes sense. I'd go with .IdFrom() .Doc() and .DocAsUpsert()

@gmarz

This comment has been minimized.

Show comment
Hide comment
@gmarz

gmarz Jul 16, 2014

Member

I'm a bit torn here.

While I like the verbosity of PartialDocument and the fact that it's more C#-ish, if you will, I still think it might be confusing to some people because they're going to be looking for the matching Doc property from the Elasticsearch DSL. The majority of the time, NEST maps 1-to-1 with the ES DSL names, so I think we should stay consistent here.

I'm +1 for .IdFrom() and Doc()

Member

gmarz commented Jul 16, 2014

I'm a bit torn here.

While I like the verbosity of PartialDocument and the fact that it's more C#-ish, if you will, I still think it might be confusing to some people because they're going to be looking for the matching Doc property from the Elasticsearch DSL. The majority of the time, NEST maps 1-to-1 with the ES DSL names, so I think we should stay consistent here.

I'm +1 for .IdFrom() and Doc()

@Mpdreamz Mpdreamz self-assigned this Jul 16, 2014

@Mpdreamz

This comment has been minimized.

Show comment
Hide comment
@Mpdreamz

Mpdreamz Jul 16, 2014

Member

Ok so thats 2-1 in favour of IdFrom() and Doc() will update the PR 👍

Member

Mpdreamz commented Jul 16, 2014

Ok so thats 2-1 in favour of IdFrom() and Doc() will update the PR 👍

@gmarz

This comment has been minimized.

Show comment
Hide comment
@gmarz

gmarz Jul 16, 2014

Member

👍

Member

gmarz commented Jul 16, 2014

👍

@gmarz

This comment has been minimized.

Show comment
Hide comment
@gmarz

gmarz Jul 16, 2014

Member

LGTM

Member

gmarz commented Jul 16, 2014

LGTM

gmarz added a commit that referenced this pull request Jul 16, 2014

@gmarz gmarz merged commit d2c5002 into develop Jul 16, 2014

1 check passed

default Finished TeamCity Build Elasticsearch.NET / NEST :: build and test : Tests passed: 992, ignored: 6
Details

@gmarz gmarz deleted the fix/streamline-updates branch Jul 16, 2014

@joehmchan

This comment has been minimized.

Show comment
Hide comment
@joehmchan

joehmchan Jul 22, 2014

I've just updated nuget to 1.0.0. There are breaking changes after ID() renamed Id(T) to IdFrom(T), PartialDocument() to Doc() and PartialDocumentAsUpsert() to DocAsUpsert().

joehmchan commented Jul 22, 2014

I've just updated nuget to 1.0.0. There are breaking changes after ID() renamed Id(T) to IdFrom(T), PartialDocument() to Doc() and PartialDocumentAsUpsert() to DocAsUpsert().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment