-
Notifications
You must be signed in to change notification settings - Fork 24.3k
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
Painless Spec Documentation Clean Up #29441
Conversation
Pinging @elastic/es-core-infra |
@@ -5,39 +5,8 @@ include::../Versions.asciidoc[] | |||
|
|||
include::painless-getting-started.asciidoc[] |
There was a problem hiding this comment.
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 nice to fix up painless-description
at some point. It talks about "alternatives" like groovy is still an alternative.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And it'd be good to clean up getting-started at some point too. Both are fine to do later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed on both. Most of the documentation needs some editing. Definitely different PRs.
*Grammar:* | ||
[source,ANTLR4] | ||
---- | ||
SINGLE_LINE_COMMENT: '//' .*? [\n\r]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you are really into including bits of the grammar you can actually include it at documentation build time with a construct like this one:
["source","ANTLR4",subs="attributes,callouts,macros"]
--------------------------------------------------
include-tagged::{parser}[comments]
--------------------------------------------------
It'd be a neat thing to do in a followup because it'd make sure the grammar never goes out of date.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will keep this in mind. Some of the grammar has been modified for the documentation and does not directly follow the current grammar precisely out of convenience for the reader.
---- | ||
// single-line comment | ||
|
||
<code> // single-line comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd probably write some actual code here, like int i = 0; // single line comment
or something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Integer literals of `int 0`, `double 0.0`, `long 1234`, | ||
`float -90.0`, `int -18` in octal, and `int 3882` in hex. | ||
|
||
[source,Java] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd probably use [source,painless]
here instead. The language doesn't really do anything at documentation rendering time. It is used more when we test the documentation. And it might be useful to have this be accurate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Will work through switching all the examples as I edit those sections.
*Examples:* | ||
[source,Java] | ||
---- | ||
_ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should document that _
is a valid variable identifier. I think Java has been trying to move away from that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed from the docs. Probably not worth removing from the language at this point given backwords compat issues, but that's a different discussion.
|
||
*Examples:* | ||
|
||
Assigning a literal of the appropriate type directly to a declared variable. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd probably replace the trailing .
with a :
because it is describing the block. I'm not sure what feels more right to be honest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to stick with standard sentence notation as it's more natural if there's more than one sentence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As long as it's consistent, either (or even none) works. I'd be inclined to drop the colons after the Grammar and Examples headings, they aren't really necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will remove the colons from example and grammar as I work through editing the sections.
|
||
[source,Java] | ||
---- | ||
ArrayList l = new ArrayList(); // Declare an ArrayList variable l and set it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one is one character too wide on my screen. The t
gets cut off.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't get to that section yet. Only edited comments, keywords, and literals so far.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I left a few thoughts.
Elasticsearch or Painless itself. Below is a list of all available | ||
classes grouped with their respected methods. Clicking on the method | ||
name takes you to the documentation for that specific method. Methods | ||
defined in the JRE also have a `(java 9)` link which can be used to see |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should consider (not here, but as a followup/separate issue) changing this to java 10, since in 6.3 we are changing to java 10 support.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, this whole thing needs to be updated, and it's actually blocking finishing up type removal. Just never had the opportunity to go back and modify it for contexts as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd mention the Java version once and avoid repeating it in every link.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well the links redirect to the Java 9 counterpart, but I agree that we need a better overall solution to keep up with the Java API moving forward. I might be inclined to only link to the last long-term release, but I'm not sure that's correct depending on what version we are technically supporting for each ES release.
===== Floats | ||
|
||
Use floating point literals to specify a floating point value of the | ||
<<primitive-types, primitive types>> float or double. Use the following single |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should float
and double
be in a monospace font (as they are below)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@debadair Do you have a bit of time to review this soon? If not, are you okay with me going with the current feedback and maybe you could come back around to more editing in the future? Thanks in advance. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some general comments. After this is merged, I can do an editing pass and open a PR.
@@ -1,17 +1,14 @@ | |||
["appendix",id="painless-api-reference"] | |||
[appendix] | |||
[[painless-api-reference]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't make this an appendix, it's fine as a regular top-level section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will remove. Was just leaving it as it was. What's the purpose of appendix in this case, does it add anything special to the docs other than the 'A'?
docs/painless/index.asciidoc
Outdated
include::painless-lang-spec.asciidoc[] | ||
|
||
include::painless-syntax.asciidoc[] | ||
include::painless-general-syntax.asciidoc[] | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd include this from painless-lang-spec.asciidoc, rather than at the top level so it reflects how things are actually nested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed, please note this is how it was before.
@@ -1,73 +1,46 @@ | |||
[[painless-specification]] | |||
[[painless-lang-spec]] | |||
== Painless Language Specification |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing the anchor text changes the name of the generated page. That's fine, but we need to keep track of the changes so we can have the webteam set up redirects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is unfortunate, but I think the changes to the documentation structure are important for our users in this case.
@@ -1,94 +0,0 @@ | |||
[[literals]] | |||
=== Literals |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moving these sections under Basic syntax also means we need a redirect, because they are no longer separate pages.
Rather than combining this into the other file, I probably would have kept it separate and included it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Separated all the sections out to separate pages after our conversation. Hopefully, this is easier to work with and better for the user.
** <<promotion, Promotion>> | ||
* <<painless-operators, Operators>> | ||
* <<painless-general-syntax, General Syntax>> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we'd be better off with a single syntax section. The "Basic" and "General" groupings aren't really going to help people know where to look.
*Examples:* | ||
|
||
String literals using both single-quotes and double-quotes. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd probably split this into "Single quoted string literals" and "Double quoted string literals" examples.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
*Examples:* | ||
|
||
Floating point literals of `double 0.0`, `double 1000000.0` in | ||
exponent notation, `double 0.977777`, `double -126.34`, and `float 89.9`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe use comments like you did in the variable declaration section?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Switched to callouts.
[source,Java] | ||
---- | ||
int i = 10; // Declare the int variable i and set it the int literal 1 | ||
double j = 2.0; // Declare the double variable j and set it to the double |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be 10?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't updated this section yet. Will do so in another PR.
// default value null | ||
m = k; // Set the value of List variable m to the value | ||
// of List variable k | ||
---- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inserting a blank line before each example would make it a little easier to read the wrapped comments. We could use callouts instead of the comments, but I think there are advantages to keeping the info inline.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Started to switch some to callouts. I really like the formatting of the callouts and think that can make it easier to follow the examples.
|
||
<code> // single-line comment | ||
This specification is divided into the following sections: | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The intro text isn't really necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed this and with the flatten structure removed the links as well as they show up in the sidebar.
@debadair Hey Deb, as per our conversation, I've updated the painless spec into a flatter structure and separate pages. Also tried to do more clean up on the 3 sections comments, keywords, and literals. Once this PR is completed, I'll move onto the next section. Thanks for all the help so far! |
Just built it & I think the flatter structure works better. I think the callouts and other changes make to those sections make it easier to read the examples. LGTM! |
* master: Remove the index thread pool (#29556) Remove extra copy in ScriptDocValues.Strings Fix full cluster restart test recovery (#29545) Fix binary doc values fetching in _search (#29567) Mutes failing MovAvgIT tests Fix the assertion message for an incorrect current version. (#29572) Fix the version ID for v5.6.10. (#29570) Painless Spec Documentation Clean Up (#29441) Add versions 5.6.10 and 6.2.5 [TEST] test against scaled value instead of fixed epsilon in MovAvgIT Remove `flatSettings` support from request classes (#29560) MapperService to wrap a single DocumentMapper. (#29511) Fix dependency checks on libs when generating Eclipse configuration. (#29550) Add null_value support to geo_point type (#29451) Add documentation about the include_type_name option. (#29555) Enforce translog access via engine (#29542)
Created a flatter structure for the different sections. Cleaned up comments, keywords, and literals. Used callouts for examples where it made sense.
First sweep of Painless spec documentation clean up. The following changes have been made: