Skip to content

Conversation

@olcbean
Copy link
Contributor

@olcbean olcbean commented Feb 16, 2018

Consistent doc title with the ES docs
Wrapping the snippets for better readability

@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

1 similar comment
@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

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.

@olcbean thanks for the reformatting, looks good in general. I left a note about also changing the tags for the code inclusion, but I'm on the fence here. Would like to hear your or others opinions. Maybe not really necessary to change, but I wanted to raise it as a suggestion.

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 curious why the sections were all called "update-aliases-*" before. The request name (IndicesAliasesRequest) doesn't seem to have changed recently. Maybe it would make sense to update the tags (and the corresponding includes) as well now?

Copy link
Member

Choose a reason for hiding this comment

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

this is on me, it's a common inconsistency between our Client method names, our request names, and how the API are called in our SPEC. We are now following the name from the SPEC when naming the new methods. But we leave requests unchanged to reduce noise and not impact bw comp too much at this stage. Let's leave the tags as they are?

Copy link
Member

Choose a reason for hiding this comment

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

Should the section shortcut also change to "indices-aliases"?

Copy link
Member

Choose a reason for hiding this comment

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

no please :) it does not matter that much, and both names are correct :)

Copy link
Member

Choose a reason for hiding this comment

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

one more inconsistency not mentioned above: the way we reference our API in our docs may differ from the request names, the client method names and the api names in our SPEC :) here we try to name pages the same as how they are named in the es reference

Copy link
Member

Choose a reason for hiding this comment

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

same here

Copy link
Member

Choose a reason for hiding this comment

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

+1, I'm in favour of changing the indentation.
Unfortunately we don't seem to have a way to catch accidental reformatting of these sections in the future. Just a remark, nothing to change here I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, we don't. The only way to do that is to use the //@formatter:off for the snippet sections but the tags are not activated by default ( at least in eclipse, but I would guess in IntelliJ is the same). I would say one more reason to consider using a simple formatter for ES ;)

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.

@javanna thanks for the clarification
@olcbean LGTM then, I will merge

@cbuescher
Copy link
Member

@elasticmachine test this please

@olcbean
Copy link
Contributor Author

olcbean commented Feb 20, 2018

@cbuescher thanks :)
@javanna after this PR is merged I will go ahead and wrap all snippets for the high-level client ?

@javanna
Copy link
Member

javanna commented Feb 20, 2018

@olcbean what do you mean with wrapping all snippets?

@olcbean
Copy link
Contributor Author

olcbean commented Feb 20, 2018

@javanna currently there are several snippets in the docs which are not "wrapped" ( nicely displayed in the generated docs ) : here, here, etc.

@cbuescher
Copy link
Member

@olcbean the failure I see in the CI run looks unrelated to your change and should be fixed on more recent versions of master. Could you rebase/merge the current master branch?

Wrapping the snippets for better readability
@olcbean olcbean force-pushed the hlrc_update_aliases_doc branch from 5bffcf3 to 594edd1 Compare February 20, 2018 12:59
@javanna
Copy link
Member

javanna commented Feb 20, 2018

@olcbean oh I see what you mean. Go ahead, agreed, thank you!

@cbuescher
Copy link
Member

@elasticmachine test this please

@cbuescher cbuescher merged commit 50d8a25 into elastic:master Feb 20, 2018
cbuescher pushed a commit that referenced this pull request Feb 20, 2018
Make doc titles consistent with the ES docs and wrapping 
the code snippets for better readability.
@cbuescher cbuescher added >docs General docs changes v7.0.0 v6.3.0 labels Feb 20, 2018
martijnvg added a commit that referenced this pull request Feb 22, 2018
* es/master: (143 commits)
  Revert "Disable BWC tests for build issues"
  Remove AcknowledgedRestListener in favour of RestToXContentListener (#28724)
  Build: Consolidate archives and packages configuration (#28760)
  Skip some plugins service tests on Windows
  Migrate some *ResponseTests to AbstractStreamableXContentTestCase (#28749)
  Disable BWC tests for build issues
  Ensure that azure stream has socket privileges (#28751)
  [DOCS] Fixed broken link.
  Pass InputStream when creating XContent parser (#28754)
  [DOCS] Changed to use transient setting to reenabled allocation. Closes #27677
  Delay path expansion on Windows
  [TEST] replace randomAsciiAlphanumOfLengthBetween with randomAsciiLettersOfLengthBetween
  [Tests] Extract the testing logic for Google Cloud Storage (#28576)
  [Docs] Update links to java9 docs (#28750)
  version set in ingest pipeline (#27573)
  Revert "Add startup logging for standalone tests"
  Tests: don't wait for completion while trying to get completed task
  Add 5.6.9 snapshot version
  [Docs] Java high-level REST client : clean up (#28703)
  Updated distribution outputs in contributing docs
  ...
sebasjm pushed a commit to sebasjm/elasticsearch that referenced this pull request Mar 10, 2018
Make doc titles consistent with the ES docs and wrapping 
the code snippets for better readability.
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.

5 participants