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

Docs: HighLevelRestClient#exists #29073

Merged
merged 4 commits into from
Mar 15, 2018
Merged

Conversation

nik9000
Copy link
Member

@nik9000 nik9000 commented Mar 14, 2018

Add documentation for HighLevelRestClient#exists.

Relates to #28389

nik9000 added 2 commits March 14, 2018 15:40
Add documentation for `HighLevelRestClient#exists`.

Relates to elastic#28389
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@olcbean olcbean mentioned this pull request Mar 14, 2018
4 tasks
Copy link
Member

@cbuescher cbuescher left a comment

Choose a reason for hiding this comment

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

@nik9000 left a tiny nit, otherwise LGTM

[[java-rest-high-document-exists-request]]
==== Exists Request

The `exists()` API is `true` if a document exists, and `false` otherwise.
Copy link
Member

Choose a reason for hiding this comment

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

nit: is -> returns

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm... In most docs there is no explanation what the API is for ( just how to use it )
Should this be moved into the above section?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I should move it to the section above.

I can use returns instead of is, sure. I wanted to use is because async calls don't really return, but is is much less clear. returns is almost certainly better.

The `exists()` API is `true` if a document exists, and `false` otherwise.
It uses `GetRequest` just like the <<java-rest-high-document-get>>.
All of its <<java-rest-high-document-get-request-optional-arguments, optional arguments>>
are supported. Since `exists()` only returns `true` or false, we recommend
Copy link
Member

Choose a reason for hiding this comment

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

false wrapped with backticks as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

++

include-tagged::{doc-tests}/CRUDDocumentationIT.java[exists-execute-async]
--------------------------------------------------
<1> The `GetRequest` to execute and the `ActionListener` to use when
the execution completes
Copy link
Member

Choose a reason for hiding this comment

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

. at the end of the sentence?

Copy link
Member Author

Choose a reason for hiding this comment

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

++

Copy link
Member

@javanna javanna left a comment

Choose a reason for hiding this comment

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

LGTM thanks @nik9000 <3

Copy link
Contributor

@olcbean olcbean left a comment

Choose a reason for hiding this comment

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

@nik9000 Thank you for writing the docs 👍
Highlighting a few minor nits that jumped out at me.

// tag::exists-request
GetRequest getRequest = new GetRequest(
"posts", // <1>
"doc", // <2>
Copy link
Contributor

Choose a reason for hiding this comment

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

would "_doc" be better here ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think it makes a difference to be honest.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, it does not ;)
I was thinking of #27816 and #27751 ( Docs should use _doc as a type name whenever possible ). I do not think that it is consistently done in the docs for the high level client though. Just a thought ...

[[java-rest-high-document-exists-request]]
==== Exists Request

The `exists()` API is `true` if a document exists, and `false` otherwise.
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm... In most docs there is no explanation what the API is for ( just how to use it )
Should this be moved into the above section?

[[java-rest-high-document-exists-async]]
==== Asynchronous Execution

The asynchronous execution of `exists()` requires both the `GetRequest`
Copy link
Contributor

Choose a reason for hiding this comment

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

In most docs this section starts something like :
Update API

The asynchronous execution of an update request requires both the UpdateRequest instance and an ActionListener instance to be passed to the asynchronous method:

Would it be more consistent if :
s/exists()/exists request/

Copy link
Member Author

Choose a reason for hiding this comment

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

I can do that. I'm not sure it is better, but it is more consistent.

<2> Type
<3> Document id
<4> Disable fetching `_source`.
<5> Disable fetching stored fields.
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm... I would say with . or without . for all? What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

I put the . because these are sentences and the ones above aren't.

@nik9000 nik9000 merged commit cf60e93 into elastic:master Mar 15, 2018
@nik9000
Copy link
Member Author

nik9000 commented Mar 15, 2018

Thanks for reviewing @javanna, @cbuescher, and @olcbean! This is a fairly good introduction to the high level rest client for me so I'm quite happy to do it!

nik9000 added a commit that referenced this pull request Mar 15, 2018
Docs: HighLevelRestClient#exists

Add documentation for `HighLevelRestClient#exists`.

Relates to #28389
martijnvg added a commit that referenced this pull request Mar 16, 2018
* es/6.x: (89 commits)
  Clarify requirements of strict date formats. (#29090)
  Clarify that dates are always rendered as strings. (#29093)
  [Docs] Fix link to Grok patterns (#29088)
  Fix starting on Windows from another drive (#29086)
  Use removeTask instead of finishTask in PersistentTasksClusterService (#29055)
  Added minimal docs for reindex api in java-api docs
  Allow overriding JVM options in Windows service (#29044)
  Clarify how to set compiler and runtime JDKs (#29101)
  Fix Parsing Bug with Update By Query for Stored Scripts (#29039)
  TEST: write ops should execute under shard permit (#28966)
  [DOCS] Add X-Pack upgrade details (#29038)
  Revert "Improve error message for installing plugin (#28298)"
  Docs: HighLevelRestClient#exists (#29073)
  Validate regular expressions in dynamic templates. (#29013)
  [Tests] Fix GetResultTests and DocumentFieldTests failures (#29083)
  Reenable LiveVersionMapTests.testRamBytesUsed on Java 9. (#29063)
  Mute failing GetResultTests and DocumentFieldTests
  [Docs] Fix Java Api index administration usage (#28260)
  Improve error message for installing plugin (#28298)
  Decouple XContentBuilder from BytesReference (#28972)
  ...
martijnvg added a commit that referenced this pull request Mar 16, 2018
* es/master: (97 commits)
  Clarify requirements of strict date formats. (#29090)
  Clarify that dates are always rendered as strings. (#29093)
  Compilation fix for #29067
  [Docs] Fix link to Grok patterns (#29088)
  Store offsets in index prefix fields when stored in the parent field (#29067)
  Fix starting on Windows from another drive (#29086)
  Use removeTask instead of finishTask in PersistentTasksClusterService (#29055)
  Added minimal docs for reindex api in java-api docs
  Allow overriding JVM options in Windows service (#29044)
  Clarify how to set compiler and runtime JDKs (#29101)
  CLI: Close subcommands in MultiCommand (#28954)
  TEST: write ops should execute under shard permit (#28966)
  [DOCS] Add X-Pack upgrade details (#29038)
  Revert "Improve error message for installing plugin (#28298)"
  Docs: HighLevelRestClient#exists (#29073)
  Validate regular expressions in dynamic templates. (#29013)
  [Tests] Fix GetResultTests and DocumentFieldTests failures (#29083)
  Reenable LiveVersionMapTests.testRamBytesUsed on Java 9. (#29063)
  Mute failing GetResultTests and DocumentFieldTests
  Improve error message for installing plugin (#28298)
  ...
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.

6 participants