-
Notifications
You must be signed in to change notification settings - Fork 13
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
Add variable type selector to optimize API usage #148
Conversation
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.
Overall looks good! Add some somments
src/language_provider.ts
Outdated
|
||
const data: string[] = await this.request(url, [], urlParams); | ||
// Convert string array to Record<string , []> | ||
return data.reduce((ac, a) => ({ ...ac, [a]: '' }), {}); |
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.
from the comment i see that the Record
should be Record<string , []>
, but in the reduce the object is {[string]: string}
. Maybe I miss 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.
Yes, there's a mistake here. I checked, only keys are used everywhere, so it's more logical to leave it as string[]
.
if (labelNamesQuery) { | ||
return this.labelNamesQuery(); | ||
return this.datasource.getTagKeys({ filters: [], timeRange }); | ||
} | ||
|
||
const labelValuesQuery = this.query.match(labelValuesRegex); | ||
if (labelValuesQuery) { |
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 should check the length also
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.
In case of no matches, the result will be null
, not an empty array.
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!
# Conflicts: # CHANGELOG.md
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!
add variable type selector to optimize API usage, favoring /api/v1/labels and /api/v1/label/.../values over /api/v1/series.
#144