Skip to content

Conversation

@dadoonet
Copy link
Contributor

The REST Client is split into 2 parts:

  • Low level
  • High level

The High level client has a main common section and the document delete API documentation as a start.

I'd really like that we have a QA module where we test all the methods we are documenting and even better only include code snippets into the documentation so we know it's always up to date.

I'd like to start with a QA project if no one objects.

The REST Client is split into 2 parts:

* Low level
* High level

The High level client has a main common section and the document delete API documentation as a start.
@dadoonet dadoonet self-assigned this Feb 24, 2017
RestHighLevelClient client = highLevelClient();

// tag::delete-request[]
DeleteRequest request = new DeleteRequest(
Copy link
Member

Choose a reason for hiding this comment

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

I'd use normal indenting here. I think we can strip them in the magic perl script if we really want to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That'd be lovely as this formatting is a pain for now.
I'm going to change that in the source code then but can you tell me how I can fix the rendering then?

Copy link
Member

Choose a reason for hiding this comment

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

I'd commit with the indented format and fix in a followup. I'm thinking of writing a little fancier perl script in a followup.

import java.io.IOException;

/**
* This class is used to generate the Java 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.

I'd add a note about how the tags are resolved. Just because this is the first usage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed.


// tag::delete-notfound[]
if (response.getResult().equals(DocWriteResponse.Result.NOT_FOUND)) {
// <1>
Copy link
Member

Choose a reason for hiding this comment

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

Maybe throw an exception from here just so the docs look more realistic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed.


["source","java",subs="attributes,callouts"]
--------------------------------------------------
sys2::[perl -ne 'exit if /end::delete-request/; print if $tag; $tag = $tag || /tag::delete-request/' {docdir}/../../client/rest-high-level/src/test/java/org/elasticsearch/client/DocumentationIT.java]
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 is worth dumping the script into the root of the doc directory so you don't have to repeat it? On second thought, maybe just add it like it is now and I'll see about making a followup? I have some ideas - maybe write an asciidoc macro or something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's keep it simple for now but that would be lovely to have a gradle task which does that extraction for instance and generate all the doc so we don't call anymore build_docs.pl but gradle docs or something.


[source,java]
--------------------------------------------------
try {
Copy link
Member

Choose a reason for hiding this comment

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

Do you want to reference this from the file? I think it'd be nice if everything came from a file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed. Not sure why I did not write it like this... o_O

/**
* This class is used to generate the Java API documentation
*/
public class DocumentationIT extends ESRestHighLevelClientTestCase {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe DeleteDocumentationIT. Also maybe put in org.elasticsearch.client.documentation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah.


Then you have access to the high level APIs such as:

include::apis.asciidoc[]
Copy link
Member

Choose a reason for hiding this comment

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

Link instead of include here? I'm not sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found out that it gives a better documentation as the guy does not have to clic but has everything at hand.

Copy link
Member

Choose a reason for hiding this comment

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

Sure.


include::apis.asciidoc[]

Each API comes with 2 ways of executing it:
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 it'd be easier to write something like:

Each API can be executed synchronously (i.e. `delete`) or asynchronously (i.e. `deleteAysnc`).
The asynchronous APIs require a listener that is called on thread pool managed by the low level client
when the response is received.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

@nik9000
Copy link
Member

nik9000 commented Feb 27, 2017

I'd really like that we have a QA module where we test all the methods we are documenting and even better only include code snippets into the documentation so we know it's always up to date.

We could put the test in the docs build instead of into the qa module. It is functionally quite similar to the testing we do in the docs build right now.

elasticsearch. Replace the version with the desired client version, first
released with `5.x.x`. There is no relation between the client version
and the elasticsearch version that the client can communicate with as
it depends on the <<java-rest-low>> which is compatible with all elasticsearch versions.
Copy link
Member

Choose a reason for hiding this comment

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

I'm afraid this will have to be rephrased. If 5.y adds some fields to the response, only version 5.y of the high level client will support that addition. the versions are more important compared to the low level client. Maybe just stop at "Replace the version with the desired client version" ?

@dadoonet
Copy link
Contributor Author

dadoonet commented Mar 1, 2017

@nik9000 @javanna Thanks guys for your comments. I pushed new changes. LMK

import java.io.IOException;

/**
* 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.

Add a . to the end of this line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. I also fixed a missing parenthesis in code...


Then you have access to the high level APIs such as:

include::apis.asciidoc[]
Copy link
Member

Choose a reason for hiding this comment

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

Sure.

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.

Looks fine to me. The docs don't compile though.

@nik9000
Copy link
Member

nik9000 commented Mar 1, 2017

The docs don't compile though.

I believe it is hitting the example tag at the top instead of lower.

/**
* This class is used to generate the Java Delete API documentation.
* You need to wrap your code between two tags like:
* // tag::delete-request[]
Copy link
Member

Choose a reason for hiding this comment

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

Change this to // tag:example[]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lol!

@dadoonet
Copy link
Contributor Author

dadoonet commented Mar 1, 2017

I'm going to fix this bad rendering as well...

image

@dadoonet
Copy link
Contributor Author

dadoonet commented Mar 1, 2017

Actually the rendering is "bad" because of the indentation. If we fix indentation, this problem will go away.

@dadoonet dadoonet merged commit dc80a1f into elastic:master Mar 1, 2017
@dadoonet dadoonet deleted the doc/hlclient-delete branch March 1, 2017 12:40
@dadoonet
Copy link
Contributor Author

dadoonet commented Mar 1, 2017

Thanks @nik9000. Merged in master.

@javanna
Copy link
Member

javanna commented Mar 1, 2017

thanks @dadoonet ! Once the high level rest client is ready for its first release, we will backport it to 5.x, and that will simply mean copying the high-level folder in a single commit. We will also have to backport the docs, but I see that this PR changes some things outside of the high-level directory, and I understand there will be subsequent changes going in. I wonder how complicated it will be come to backport the docs when we need to. Any ideas on that?

@dadoonet
Copy link
Contributor Author

dadoonet commented Mar 1, 2017

I don't think it will be a big deal as I don't recall having conflicts or anything like that in docs. I believe we can cherry-pick one or more of my commits from this PR.

@javanna
Copy link
Member

javanna commented Mar 1, 2017

sure @dadoonet that will mean that we need to take note of these commits and all the following ones on the matter. This is something highly likely to cause mess ups I'm afraid, I will be happy to be proven wrong :)

@dadoonet
Copy link
Contributor Author

dadoonet commented Mar 1, 2017

What is the other solution you have in mind?
Having doc alongside code? So if you have doc in client/rest-high-level/doc then it will be part or the copy/paste you mentioned?

@javanna
Copy link
Member

javanna commented Mar 1, 2017

I do not have another solution in mind, let's just keep this in mind for the future changes and try to keep them isolated to the high-level folder as much as possible.

dadoonet added a commit to elastic/docs that referenced this pull request Mar 1, 2017
As we are extracting our documentation from the code since elastic/elasticsearch#23351 we need to add the path to the source code when generating the documentation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

>docs General docs changes v6.0.0-alpha1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants