-
Notifications
You must be signed in to change notification settings - Fork 8.5k
[ES|QL] Displays The Command License Availability In Our Inline Docs #230358
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
[ES|QL] Displays The Command License Availability In Our Inline Docs #230358
Conversation
|
Pinging @elastic/kibana-esql (Team:ESQL) |
6f3a1f2 to
102fec0
Compare
| const n = recast.types.namedTypes; | ||
| import fs from 'fs'; | ||
| import path from 'path'; | ||
| import { functions } from '../src/sections/generated/scalar_functions'; |
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 noticed that this existing script updates an existing functions file. However, if a file in the generated folder is missing or becomes corrupted, the script crashes. We could improve this behavior with a small change —either in this PR, in a separate one, or leave it as is for now, if it is not a relevant case.
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.
Yeah I had also noticed it, it is a case that is not happening in reality (unless you deliberately add a new file) so I don't think it is very important to fix
1dfdd29 to
93dec68
Compare
| labelKey: string; | ||
| descriptionKey: string; | ||
| } { | ||
| const labelKeySuffixMap: Record<string, string> = { |
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 not a big fan of these bindings, but the only alternative is to change the names of the translation keys in the translation json files (and probably remove this function)
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.
Yeah I am not in favor of this either. Why are you doing this? Is this about converting stings like aaa_bbb to aaaBBB?? Why don't you use the camelCase of lodash?
If the keys are different now that we automated is better to update them with the automated ones rather than hardcode them in the script.
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 need to handle cases where the label keys in the translation files don’t match the command names exactly. For example, a label key might be "languageDocumentation.documentationESQL.lookupJoin", but the corresponding commandName is simply lookup.
In most cases, the label keys and command names follow a predictable pattern and can be generated programmatically. However, there are exceptions like the one above, where a manual mapping is required to ensure accuracy.
As part of this, I’ll be updating the keys in the translation files to align with these mappings.
93dec68 to
7603b4b
Compare
stratoula
left a 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.
This looks very nice already, I left some comments
| const n = recast.types.namedTypes; | ||
| import fs from 'fs'; | ||
| import path from 'path'; | ||
| import { functions } from '../src/sections/generated/scalar_functions'; |
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.
Yeah I had also noticed it, it is a case that is not happening in reality (unless you deliberately add a new file) so I don't think it is very important to fix
| "make:docs": "./scripts/make_docs.sh", | ||
| "postmake:docs": "yarn run lint:fix", | ||
| "lint:fix": "cd ../../../../.. && node ./scripts/eslint --fix ./src/platform/packages/private/kbn-language-documentation/src/sections/generated" | ||
| "lint:fix": "cd ../../../../.. && node ./scripts/eslint --fix ./src/platform/packages/private/kbn-language-documentation/src/sections/generated ./src/platform/packages/private/kbn-language-documentation/src/sections/esql_documentation_sections.tsx" |
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.
Why this change??
| 'commands' | ||
| ); | ||
|
|
||
| interface CommandData { |
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 have to admin that these interface having similar names is confusing me a bit. Can we give more descriptive named here?
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 CommandTemplateData
these are the object parameters we use to build the final doc
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.
yes this sounds better!
| /** | ||
| * Generates a documentation for a specific group of commands. | ||
| */ | ||
| function generateCommandsDoc({ |
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.
These 2 functions the same
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 have a function called generateCommandsDoc and one called generateCommandDoc
Are these the two functions we are talking about? Eventually I can change the name as generateCommandItemDoc
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.
yes these 2, almost same names so it would be cool if they can be more descriptive.. This generateCommandItemDoc sounds much better ++
| labelKey: string; | ||
| descriptionKey: string; | ||
| } { | ||
| const labelKeySuffixMap: Record<string, string> = { |
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.
Yeah I am not in favor of this either. Why are you doing this? Is this about converting stings like aaa_bbb to aaaBBB?? Why don't you use the camelCase of lodash?
If the keys are different now that we automated is better to update them with the automated ones rather than hardcode them in the script.
stratoula
left a 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.
Awesome, LGTM! I didnt test it again, only code review
💚 Build Succeeded
Metrics [docs]Module Count
Async chunks
Unknown metric groupsESLint disabled in files
Total ESLint disabled count
History
cc @bartoval |
…lastic#230358) ## Summary This PR is part of elastic#216791 Provide iDocs badges for commands with license. - Creates a script to generate a command documentation file. - Creates a new command template file that can be used as a starting point for defining new commands and generating their documentation. - Clean up the original doc file - Small refactoring to share utils and types examples: <img width="442" height="453" alt="change_point" src="https://github.com/user-attachments/assets/9da2f0a6-d0b9-4f38-bc2a-3857c94cbe2c" /> --------- Co-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co>
…lastic#230358) ## Summary This PR is part of elastic#216791 Provide iDocs badges for commands with license. - Creates a script to generate a command documentation file. - Creates a new command template file that can be used as a starting point for defining new commands and generating their documentation. - Clean up the original doc file - Small refactoring to share utils and types examples: <img width="442" height="453" alt="change_point" src="https://github.com/user-attachments/assets/9da2f0a6-d0b9-4f38-bc2a-3857c94cbe2c" /> --------- Co-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co>
…lastic#230358) ## Summary This PR is part of elastic#216791 Provide iDocs badges for commands with license. - Creates a script to generate a command documentation file. - Creates a new command template file that can be used as a starting point for defining new commands and generating their documentation. - Clean up the original doc file - Small refactoring to share utils and types examples: <img width="442" height="453" alt="change_point" src="https://github.com/user-attachments/assets/9da2f0a6-d0b9-4f38-bc2a-3857c94cbe2c" /> --------- Co-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co>
Summary
This PR is part of #216791
Provide iDocs badges for commands with license.
examples: