-
Notifications
You must be signed in to change notification settings - Fork 8.5k
[ES|QL] Displays The Function License Availability In Our Inline Docs #229961
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 Function License Availability In Our Inline Docs #229961
Conversation
|
@florent-leborgne |
src/platform/packages/shared/kbn-esql-ast/src/definitions/utils/documentation.ts
Show resolved
Hide resolved
|
|
||
| function createLicenseTooltip(license: LicenseInfo): string { | ||
| let tooltip = i18n.translate('languageDocumentation.licenseRequiredTooltip', { | ||
| defaultMessage: 'This feature requires {license} license', |
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.
Are internationalization files modified automatically? I don't know Mandarin or Japanese
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.
Shame!
They are being taken care of from the translations team!
|
Pinging @elastic/kibana-esql (Team:ESQL) |
ebd7e80 to
6ba0e37
Compare
| "@kbn/i18n", | ||
| "@kbn/test-jest-helpers", | ||
| "@kbn/shared-ux-markdown", | ||
| "@kbn/esql-ast", |
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.
Do we want to avoid this? I need it to add the right types instead of any
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 CI might complain actually, let's see how it goes
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.
So it didnt. Regardless I would like to avoid it to keep the package as simple as possible I think. 🤔 Mostly because you added them only for the script.
As you added it only for the script I think it is ok to create these interfaces again here and only the paths. You mostly want the license property. I don't have a strong opinion but I think if we can avoid the kbn-ast dependency would be better
|
|
||
| function createLicenseTooltip(license: LicenseInfo): string { | ||
| let tooltip = i18n.translate('languageDocumentation.licenseRequiredTooltip', { | ||
| defaultMessage: 'This feature requires {license} license', |
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.
Super ideally these sentences would say:
"This feature requires a Platinum subscription."
"This feature requires an Enterprise subscription."
- We should use "subscription" instead of "license" (that's the term we use on the website and in the product UI)
- It feels a lot more natural with "a" or "an" before the subscription name. Is this possible to add this logic?
- Can we put subscription names in title case? (not full caps, just with the first letter capitalized - in the tooltip I mean, I think it's forced to full caps for the badge label anyways)
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.
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.
Looks great, just some small comments about readability and I think I prefer to avoid the kbn-ast dependency for this package
| * Aggregates licenses from an array of signatures into a map. | ||
| * The map's key is the license name, and the value is a Set of associated parameter types. | ||
| */ | ||
| function aggregateLicensesFromSignatures(signatures: Signature[]): Map<string, Set<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.
Can we move this function and the next one to a utils file? It would be easier to read the script. The getLicenseInfo too with the types too
|
|
||
| let tooltip = i18n.translate('languageDocumentation.licenseRequiredTooltip', { | ||
| defaultMessage: | ||
| 'This feature requires {articleType, select, vowel {an} other {a}} {license} subscription', |
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.
So react-intl doesnt handle the indefinite article and we have to do it on our own?
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.
It seems it doesnt :(
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 assume this won’t be the only case where we need this. Why don’t we create a utility component based on formatMessage that handles this kind of usage?.
We will use it for ESQL but it could potentially be extended into a more general-purpose component within Kibana.
I’m thinking of something simple—just a few lines—that encapsulates this pattern
<FormattedMessageWithArticleType id="some.message" defaultMessage="This report includes [article]{userType}[/article] and [article]{eventType}[/article]." values={{ userType: 'administrator', eventType: 'error', }} />
with this format we parse [article] and manage the logic
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 I think this is a good idea but as this is a territory of another team let's proceed with this PR as it is for now.
| } | ||
|
|
||
| // Helper function to get licenses array from either format | ||
| function getLicensesArray(license: MultipleLicenseInfo | undefined): LicenseInfo[] { |
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.
Can we also move all these functions inside the utils folder?
| "@kbn/i18n", | ||
| "@kbn/test-jest-helpers", | ||
| "@kbn/shared-ux-markdown", | ||
| "@kbn/esql-ast", |
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.
So it didnt. Regardless I would like to avoid it to keep the package as simple as possible I think. 🤔 Mostly because you added them only for the script.
As you added it only for the script I think it is ok to create these interfaces again here and only the paths. You mostly want the license property. I don't have a strong opinion but I think if we can avoid the kbn-ast dependency would be better
28ffc5c to
aa7c559
Compare
| } | ||
|
|
||
| let tooltip = i18n.translate('languageDocumentation.licenseRequiredTooltip', { | ||
| defaultMessage: 'This feature requires {articleType} {license} subscription', |
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 simplified the logic
ebb4f32 to
83ad8f1
Compare
florent-leborgne
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.
Copy LGTM, thank you for the changes 🙏
|
|
||
| if (license.isSignatureSpecific && license?.paramsWithLicense?.length) { | ||
| tooltip += ` ${i18n.translate('languageDocumentation.licenseParamsNote', { | ||
| defaultMessage: ' only for specific values: {params}', |
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.
| defaultMessage: ' only for specific values: {params}', | |
| defaultMessage: ' to use the following values: {params}', |
Suggestion, feel free to ignore if you think it doesn't work for the various possible scenarios
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.
83ad8f1 to
6a636ca
Compare
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Module Count
Async chunks
Page load bundle
History
cc @bartoval |
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.
Now it is a beauty and you added tests too 😍 LGTM!
…elastic#229961) ## Summary This PR is part of elastic#216791 Provide iDocs badges for functions or function-signatures with license examples: CATEGORIZE (license at function level) <img width="639" height="459" alt="doc_categorize" src="https://github.com/user-attachments/assets/80abcbd3-f80c-4ff3-9f49-e966390be666" /> ST_EXTENT_AGG (licence at signature level) <img width="704" height="626" alt="doc_st_extent_agg" src="https://github.com/user-attachments/assets/c5ad8db3-92f9-42bb-a9e8-a55101ccb6b4" /> **Note: We don't yet know if there can be signatures with multiple different licenses, but the script supports this possibility** This plus does not bring any disadvantages so it is worth having it ready. --------- Co-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co>




Summary
This PR is part of #216791
Provide iDocs badges for functions or function-signatures with license
examples:

CATEGORIZE (license at function level)
ST_EXTENT_AGG (licence at signature level)
Note: We don't yet know if there can be signatures with multiple different licenses, but the script supports this possibility
This plus does not bring any disadvantages so it is worth having it ready.