Skip to content

Try to streamline the update api's #795

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

Merged
merged 3 commits into from
Jul 16, 2014
Merged

Try to streamline the update api's #795

merged 3 commits into from
Jul 16, 2014

Conversation

Mpdreamz
Copy link
Member

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

@Mpdreamz Mpdreamz mentioned this pull request Jul 16, 2014
@Mpdreamz
Copy link
Member Author

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
Copy link
Contributor

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
Copy link

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
Copy link
Member Author

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
Copy link

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

@gmarz
Copy link
Contributor

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
Copy link
Member Author

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

@gmarz
Copy link
Contributor

gmarz commented Jul 16, 2014

👍

@gmarz
Copy link
Contributor

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
@gmarz gmarz deleted the fix/streamline-updates branch July 16, 2014 18:06
@joehmchan
Copy link

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants