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: Clarify html encoder #27766

Merged
merged 2 commits into from
Jan 24, 2018
Merged

docs: Clarify html encoder #27766

merged 2 commits into from
Jan 24, 2018

Conversation

robinst
Copy link
Contributor

@robinst robinst commented Dec 12, 2017

The previous description was a bit confusing because the pre/post tags used for highlighting are not escaped, the rest of the content is.

The previous description was a bit confusing because the pre/post tags used for highlighting are not escaped, the rest of the content is.
@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?

@cbuescher cbuescher added >docs General docs changes :Search Relevance/Highlighting How a query matched a document v7.0.0 v6.3.0 labels Jan 23, 2018
@cbuescher cbuescher self-assigned this Jan 23, 2018
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.

@robinst sorry for the long silence on this, it seems this PR somehow fell through the cracks. I just checked the encoder behaviour and also think the current description is misleading. I left two small comment how I think the PR could be changed, let me know if you agree and want to adjust this slightly.

@@ -137,7 +137,7 @@ boundary_scanner_locale:: Controls which locale is used to search for sentence
and word boundaries.

encoder:: Indicates if the highlighted text should be HTML encoded:
Copy link
Member

Choose a reason for hiding this comment

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

I just checked that for "html" not only text between highlight tags gets escaped, but the whole snippet. I think you could also change "text" here to "text snippet" or just "snippet". Would that make more sense?

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 it's a bit confusing because "highlighted text" could also refer to all the text that is being highlighted (as opposed to the text between the highlighting tags).

@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?

@robinst
Copy link
Contributor Author

robinst commented Jan 23, 2018

@cbuescher: I've included your suggestion and reworded it a bit more.

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.

@robinst thanks for the change, reads great now I think. I will merge this after kicking off some final tests

@cbuescher
Copy link
Member

@elasticmachine test this please

@cbuescher
Copy link
Member

Unfortunately the branch is a bit old, so CI fails because we now use the gradle wrapper. However, I checked the docs build manually and since this doesn't change code or involves any code snippet tests I will merge it to avoid having to go through a full rebase & test cycle.

@cbuescher cbuescher merged commit 64bbb3a into elastic:master Jan 24, 2018
cbuescher pushed a commit that referenced this pull request Jan 24, 2018
The previous description was a bit confusing because the pre/post tags used for highlighting are not escaped, the rest of the content is.
cbuescher pushed a commit that referenced this pull request Jan 24, 2018
The previous description was a bit confusing because the pre/post tags used for highlighting are not escaped, the rest of the content is.
cbuescher pushed a commit that referenced this pull request Jan 24, 2018
The previous description was a bit confusing because the pre/post tags used for highlighting are not escaped, the rest of the content is.
cbuescher pushed a commit that referenced this pull request Jan 24, 2018
The previous description was a bit confusing because the pre/post tags used for highlighting are not escaped, the rest of the content is.
@cbuescher
Copy link
Member

@robinst thanks again and sorry for the wait. I merged this to master and all the 6.x branches, will be online like this soon.

martijnvg added a commit that referenced this pull request Jan 25, 2018
* es/master:
  [Docs] Fix explanation for `from` and `size` example (#28320)
  Adapt bwc version after backport #28358
  Always return the after_key in composite aggregation response (#28358)
  Adds test name to MockPageCacheRecycler exception (#28359)
  Adds a note in the `terms` aggregation docs regarding pagination (#28360)
  [Test] Fix DiscoveryNodesTests.testDeltas() (#28361)
  Update packaging tests to work with meta plugins (#28336)
  Remove Painless Type from MethodWriter in favor of Java Class. (#28346)
  [Doc] Fixs typo in reverse-nested-aggregation.asciidoc (#28348)
  Reindex: Shore up rethrottle test
  Only assert single commit iff index created on 6.2
  isHeldByCurrentThread should return primitive bool
  [Docs] Clarify `html` encoder in highlighting.asciidoc (#27766)
  Fix GeoDistance query example (#28355)
  Settings: Introduce settings updater for a list of settings (#28338)
  Adapt bwc version after backport #28310
martijnvg added a commit that referenced this pull request Jan 25, 2018
* es/6.x:
  [Docs] Fix explanation for `from` and `size` example (#28320)
  Adapt bwc version after backport #28358
  Always return the after_key in composite aggregation response (#28358)
  Adds a note in the `terms` aggregation docs regarding pagination (#28360)
  Update packaging tests to work with meta plugins (#28336)
  Remove Painless Type from MethodWriter in favor of Java Class. (#28346)
  [Doc] Fixs typo in reverse-nested-aggregation.asciidoc (#28348)
  [Docs] Fixed Indices information breaking changes (#27914)
  Reindex: Shore up rethrottle test
  isHeldByCurrentThread should return primitive bool
  [Docs] Clarify `html` encoder in highlighting.asciidoc (#27766)
  Only assert single commit iff index created on 6.2
  Deprecate the `update_all_types` option. (#28284)
  Fix GeoDistance query example
  Settings: Introduce settings updater for a list of settings (#28338)
  [Docs] Fix asciidoc style in composite agg docs
  Adapt bwc version after backport #28310
  Adds the ability to specify a format on composite date_histogram source (#28310)
jasontedor added a commit to matarrese/elasticsearch that referenced this pull request Jan 25, 2018
* master: (23 commits)
  Update Netty to 4.1.16.Final (elastic#28345)
  Fix peer recovery flushing loop (elastic#28350)
  REST high-level client: add support for exists alias (elastic#28332)
  REST high-level client: move to POST when calling API to retrieve which support request body (elastic#28342)
  Add Indices Aliases API to the high level REST client (elastic#27876)
  Java Api clean up: remove deprecated `isShardsAcked` (elastic#28311)
  [Docs] Fix explanation for `from` and `size` example (elastic#28320)
  Adapt bwc version after backport elastic#28358
  Always return the after_key in composite aggregation response (elastic#28358)
  Adds test name to MockPageCacheRecycler exception (elastic#28359)
  Adds a note in the `terms` aggregation docs regarding pagination (elastic#28360)
  [Test] Fix DiscoveryNodesTests.testDeltas() (elastic#28361)
  Update packaging tests to work with meta plugins (elastic#28336)
  Remove Painless Type from MethodWriter in favor of Java Class. (elastic#28346)
  [Doc] Fixs typo in reverse-nested-aggregation.asciidoc (elastic#28348)
  Reindex: Shore up rethrottle test
  Only assert single commit iff index created on 6.2
  isHeldByCurrentThread should return primitive bool
  [Docs] Clarify `html` encoder in highlighting.asciidoc (elastic#27766)
  Fix GeoDistance query example (elastic#28355)
  ...
@robinst robinst deleted the patch-1 branch February 8, 2018 04:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants