-
Notifications
You must be signed in to change notification settings - Fork 25.2k
Painless: Restructure Spec Documentation #31013
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
Conversation
examples are used.
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.
Left some comments--where applicable, the changes can be carried forward to the other sections, I didn't repeat them in each case.
|
||
The following tables list all allowed casts read as row (original type) to | ||
column (target type) with implicit as `I`, explicit as `E`, and not allowed as | ||
`-`: |
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 up. "The following tables show all allowed casts. Read the tables row by row, where the original type is shown in the first column, and each subsequent column indicates whether a cast to the specified type can be implicit (I), explicit (E), or is not allowed (-)."
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.
@@ -44,7 +44,7 @@ HEX: '-'? '0' [xX] [0-9a-fA-F]+ [lL]?; | |||
<5> `int -18` in octal | |||
<6> `int 3882` in hex | |||
|
|||
[[floats]] | |||
[[float-literals]] | |||
==== Floats |
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.
Beware that changing the anchors affects any cross-refs to these 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.
Thanks for the head up. I've been pretty good about compiling the docs and looking at the format each time I change something significant.
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.
Note that I'm fairly confident nothing is referring to the headers outside the book since I've added the majority of them recently.
initialize each dimension with are specified as a comma-separated list enclosed | ||
in braces. For example, `new int[] {1, 2, 3}` creates a one dimensional `int` | ||
array with a size of 3 and the values 1, 2, and 3. | ||
|
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 and the following two paragraphs feel redundant/repetitious. I'd be inclined to remove the second two sentences from the first paragraph.
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.
==== Boolean Not | ||
|
||
Boolean not will flip a boolean value from true to false or false to true using the bang operator. The format is a bang operator followed by an expression. | ||
|
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 I'd put not
in code font or all caps to make it a bit easier to read correctly. I'd probably say: Boolean not
flips a boolean value between true
and false
and is represented by the ! (bang) operator. The format is the bang operator followed by an expression.
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.
Used caps, tried to do this consistently for all operators that it made sense for.
---- | ||
|
||
Note that def types will be assumed to be of the boolean type. Any def type evaluated at run-time that does not represent a boolean will result in an error. Non-boolean expressions will result in an error. | ||
|
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.
Generally avoid future tense. Also, runtime doesn't need to be hyphenated. I'd say "Note that def types are assumed to be of type boolean. Any def type evaluated at runtime that does not represent a boolean results in an error."
The last sentence is not clear--is that, "Preceding a non-boolean expression with the bang operator results in an error."?
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.
---- | ||
|
||
A numeric promotion may occur during a less than operation. A def type evaluated at run-time will follow the same promotion table at run-time following whatever type def represents. Non-numeric expressions will result in an error. | ||
|
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.
Same as previous.
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.
==== Boolean And | ||
|
||
Boolean and will and together two boolean expressions. If the first expression is found to be false then it is known that the result will also be false, so evaluation of the second expression will be skipped. The table below shows what the resultant boolean value will be based on the two boolean expressions. | ||
|
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 go with, "Boolean and
ANDs two boolean expressions." Is it worth adding, "and evaluates to true if both expressions are true"?
(Also corresponding changes for 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.
Changed, used caps.
==== Precedence | ||
|
||
An expression encapsulated within a *precedence operator* overrides | ||
existing precedence relationships between operators and is evaluated |
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.
Need to call out the use of parens. The use of "precedence operator" seems a bit awkward in this case--and doesn't need to be highlighted, especially on the second occurrence. I might be inclined to go with something like, "An expression encapsulated by the precedence operator (enclosed in parentheses)..."
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.
Cleaned up. I removed the format sentence from each operator given the grammar and examples. Is this something I should add back in for each operator?
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.
Also removed the bolding of the operators from all of the general section.
==== Post Increment | ||
|
||
A variable/field representing a numerical value can be possibly evaluated as part of an expression, and then increased by 1 for its respective type. The format starts with a variable name followed by a plus and ends with a plus. | ||
|
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 follow the same format used for the other operators--something like, "use the post increment operator (++) to increase the value of a variable by one after it has been evaluated. The format is the variable name followed by ++
.
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.
==== Pre Increment | ||
|
||
A variable/field representing a numerical value can be increased by 1 for its respective type, and then possibly evaluated as part of an expression. The format starts with a plus followed by a plus and ends with a variable name. | ||
|
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.
See prev 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.
Changed.
@debadair Thank you so much for the first pass! As we discussed, I've merged ALL changes to the operators section into this PR. I apologize profusely that it's so huge, I realize the most that can be done as a second pass is a broad overview and will need editing again when you have more time, but I think it makes sense to get these changes in as I consider them an large improvement over the existing docs. I did my best to apply comments from the previous pass as appropriate to the different operators, so hopefully it's better moving forward. Note - I also merged in more structural changes as I figured I may as well try to lay out the structure for the spec all at once so you don't have to request so many hyperlinks in upcoming new documentation. The sections are the following:
|
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.
A few more very minor comments. This is definitely feeling like a big improvement--nice work! Totally agree with getting this merged!
@@ -343,12 +347,12 @@ reference type to its corresponding primitive type. | |||
|
|||
Implicit boxing/unboxing occurs during the following operations: | |||
|
|||
* Conversions between a `def` type and a primitive type will be implicitly | |||
* Conversions between a `def` type and a primitive type is implicitly | |||
boxed/unboxed as necessary, though this is referred to as an implicit cast |
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.
is > are
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.
Fixed.
==== Characters | ||
|
||
A character literal cannot be specified directly. Instead, use the | ||
A character literal is not specified directly. Instead, use the | ||
<<string-character-casting, cast operator>> to convert a `String` type value |
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 would change this to "Character literals are not..." It just reads strange to me in the singular. :-)
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 and changed.
|
||
Scripts are composed of one-to-many <<painless-statements, statements>> and are | ||
run in a sandbox that determines what local variables are immediately available | ||
along with what API's are whitelisted for use. |
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.
No apostrophe necessary in APIs
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.
Fixed.
@debadair Thanks again for the feedback! Made the changes as requested. |
@debadair Thank you so much! Will commit this as soon as the build passes. I will try to keep the next rounds much, much smaller. |
@elasticmachine Please test this. |
Full restructure of the spec into new sections for operators, statements, scripts, functions, lambdas, and regexes. Split of operators into 6 sections, a table, reference, array, numeric, boolean, and general. Clean up of all operators sections. Sporadic clean up else where.
* elastic/master: (53 commits) Painless: Restructure/Clean Up of Spec Documentation (#31013) Update ignore_unmapped serialization after backport Add back dropped substitution on merge high level REST api: cancel task (#30745) Enable engine factory to be pluggable (#31183) Remove vestiges of animal sniffer (#31178) Rename elasticsearch-nio to nio (#31186) Rename elasticsearch-core to core (#31185) Move cli sub-project out of server to libs (#31184) [DOCS] Fixes broken link in auditing settings QA: Better seed nodes for rolling restart [DOCS] Moves ML content to stack-docs [DOCS] Clarifies recommendation for audit index output type (#31146) Add nio-transport as option for http smoke tests (#31162) QA: Set better node names on rolling restart tests Add support for ignore_unmapped to geo sort (#31153) Share common parser in some AcknowledgedResponses (#31169) Fix random failure on SearchQueryIT#testTermExpansionExceptionOnSpanFailure Remove reference to multiple fields with one name (#31127) Remove BlobContainer.move() method (#31100) ...
* elastic/6.x: (50 commits) Painless: Restructure/Clean Up of Spec Documentation (#31013) Add support for ignore_unmapped to geo sort (#31153) Enable engine factory to be pluggable (#31183) Remove vestiges of animal sniffer (#31178) Rename elasticsearch-core to core (#31185) Move cli sub-project out of server to libs (#31184) QA: Fixup rolling restart tests QA: Better seed nodes for rolling restart [DOCS] Fixes broken link in release notes [DOCS] Fixes broken link in auditing settings [DOCS] Moves ML content to stack-docs [DOCS] Clarifies recommendation for audit index output type (#31146) QA: Set better node names on rolling restart tests QA: Skip mysterious failing rolling upgrade tests Share common parser in some AcknowledgedResponses (#31169) Fix random failure on SearchQueryIT#testTermExpansionExceptionOnSpanFailure Remove reference to multiple fields with one name (#31127) Remove BlobContainer.move() method (#31100) [Docs] Correct minor typos in templates.asciidoc (#31167) Use ESBlobStoreRepositoryIntegTestCase to test the repository-s3 plugin (#29315) ...
This splits general-syntax into the remaining spec sections statements, scripts, functions, lambdas, and regexes.
This also splits the operators into different sections in the Painless spec to reduce the size of the original operators page from enormous to manageable. Also includes clean up of the general operators section. Subsequent operators sections will be cleaned up in different PRs.
Note this PR looks huge, but it's actually not. Most of the lines are the split of operators into different pages.