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] add docs for REST high level client index method #25501

Merged
merged 2 commits into from
Jul 3, 2017

Conversation

javanna
Copy link
Member

@javanna javanna commented Jun 30, 2017

This commit restructures the existing high level client docs, adapts the existing delete method docs and adds docs for the index method.

This commit restructures the existing high level client docs, adapts the existing delete method docs and adds docs for the index method.
Copy link
Member

@nik9000 nik9000 left a comment

Choose a reason for hiding this comment

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

LGTM

import java.util.Map;

/**
* This class is used to generate the Java Delete API documentation.
Copy link
Member

Choose a reason for hiding this comment

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

s/Delete/CRUD/

* Then in the documentation, you can extract what is between tag and end tags with
* ["source","java",subs="attributes,callouts"]
* --------------------------------------------------
* sys2::[perl -ne 'exit if /end::example/; print if $tag; $tag = $tag || /tag::example/' \
Copy link
Member

Choose a reason for hiding this comment

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

I believe this text is out of date now that we have a macro.

{
//tag::index-request-map
Map<String, Object> jsonMap = new HashMap<>();
jsonMap.put("user","kimchy");
Copy link
Member

Choose a reason for hiding this comment

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

Nit: space after ,.

.source(jsonMap); // <1>
//end::index-request-map
IndexResponse indexResponse = client.index(indexRequest);
assertEquals(indexResponse.getResult(), DocWriteResponse.Result.CREATED);
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if it'd be nice to have the assertion in the docs with a note about how this is the response. And maybe a note that you don't have to assert anything if you don't care whether or not it was created up updated because non-200s throw exceptions.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, is that true about the non-200s? It is with the low level client.

Copy link
Member

Choose a reason for hiding this comment

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

And it looks like you cover the response below. So you can ignore this.

Copy link
Member Author

Choose a reason for hiding this comment

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

it's a good question. I'm not incredibly happy yet with what I have, but here is my thinking: I want to have some assertions, just to make sure that things really work the way we document them. Yet, I think assertions are confusing if they are made part of the docs. As a result, the docs that get published contain some ifs without instructions etc. which are not nice, but it is really up to users what to do from there...

IndexRequest request = new IndexRequest(
"index", // <1>
"type", // <2>
"id"); // <3>
Copy link
Member

Choose a reason for hiding this comment

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

Nit: alignment.

Copy link
Member

@tlrx tlrx 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 for doing this documentation effort

@javanna javanna merged commit 99fef24 into elastic:master Jul 3, 2017
javanna added a commit that referenced this pull request Jul 3, 2017
This commit restructures the existing high level client docs, adapts the existing delete method docs and adds docs for the index method.
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Jul 4, 2017
* master: (52 commits)
  Include shared/attributes.asciidoc from docs master
  Fixed page breaks for ICU Collation Keyword Fields
  Remove QueryParseContext (elastic#25486)
  [Test] Use a common testing class for all XContent filtering tests (elastic#25491)
  Tests fix - Significant terms/text aggs  (elastic#25499)
  [DOCS] add docs for REST high level client index method (elastic#25501)
  Tests: Add Debian 9 (Stretch) to the packaging tests
  test: Run flush before upgrade and refresh after upgrade.
  Fix third party audit for repository-hdfs
  [TEST] Expect nodes getting disconnected quickly
  testPrimaryFailureIncreasesTerm should use assertBusy to wait for yellow
  Cleanup network / transport related settings (elastic#25489)
  Fix repository-hdfs plugin packaging test
  Remove allocation id from replica replication response (elastic#25488)
  Adjust BWC version on bad allocation request test
  Upgrading HDFS Repository Plugin to use HDFS 2.8.1 Client (elastic#25497)
  Adjust status on bad allocation explain requests
  Preliminary support for ARM
  Add doc note regarding explicit publish host
  Fix typo in name of test
  ...
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.

4 participants