Use codeinclude for Elasticsearch Module #2828
Conversation
use codeinclude for the elasticsearch module documentation
| assertEquals(ClusterHealthStatus.GREEN, healths.getStatus()); | ||
|
|
||
| // Stop the container. | ||
| container.stop(); |
There was a problem hiding this comment.
please use try-with-resources to make sure that stop() is executed even if some assert fails
There was a problem hiding this comment.
done, but this changed the code in the documentation
There was a problem hiding this comment.
- please do not resolve conversations started by others.
- Judging by other codeincludes, it is possible to do it without changing the semantics in the docs. Please have a look at other codeincludes for some inspiration.
There was a problem hiding this comment.
First, sorry didnt know that.
Second, what i meant is, previously the docs showed container stopping with stop:
Now, the code uses try-with-resource statements which is logically different, i just wanted to point this out. If you could give me an example how to show the stop method and using try-with-resource statements in the source code, it would help me a lot. Currently the docs would look like this:
|
I was wondering if it would make more sense to use the code of the existing tests. I think they more or less show the usage for each case. The asserts would make it a bit more verbose though. This comes from the fact, that I am reluctant to add more heavy automated tests to our pipeline if they basically duplicate existing ones. |
modify container tests to reflect the code previously embedded in the documentation
I tried to merge the existing code of the tests with the code previously embedded in the documentation. i appreciate feedback on how well the new test code reflects the ideas of the original documentation code. |
the current version of elasticsearch should be used in the tests. To achieve this, the version can be determined by the version of the elasticsearch library which is included as a dependency.
| Response response = client.performRequest(new Request("GET", "/_cluster/health")); | ||
| assertThat(response.getStatusLine().getStatusCode(), is(200)); | ||
| assertThat(EntityUtils.toString(response.getEntity()), containsString("cluster_name")); |
There was a problem hiding this comment.
We should probably omit these assertions from the docs...
There was a problem hiding this comment.
is it possible to exclude lines from a code block with the codeinclude macro?
This would be really helpful feature to decrease the amount of new code in the tests
There was a problem hiding this comment.
Not directly to exclude, but you can use the same block ID more than once and every block should be included. e.g. given:
// someBlock {
A
B
// }
C
// someBlock {
D
// }
When rendered via codeinclude, it should appear like:
A
B
D
I'm partly tempted to add explicit excludes support to the codeinclude macro, but I think I'd prefer to try the above approach first.
There was a problem hiding this comment.
thanks that is exactly what i need
There was a problem hiding this comment.
This leads to a wrong indentation:
There was a problem hiding this comment.
This PR could help to solve the problem: rnorth/mkdocs-codeinclude-plugin#14
There was a problem hiding this comment.
I've been trying to untangle the PRs on the codeinclude plugin, because I think you're right. Unfortunately they're not independent, and that PR is built upon other PRs that have issues.
I think it'll be a bit of time before that gets merged.
Shall we just go ahead and merge? The indentation issue should get sorted out separately later 😄
There was a problem hiding this comment.
sounds like a plan :)
raynigon
force-pushed
the
master
branch
4 times, most recently
from
ed13b22
to
fa31aab
Compare
use codeinclude for the elasticsearch module documentation
more details: #1158