move to cmn v0.12.0 #50
Conversation
Signed-off-by: Augustin Husson <husson.augustin@gmail.com>
| ...lintKeymap, | ||
| ]), | ||
| ]; | ||
|
|
There was a problem hiding this comment.
Just want to say that making the example simpler is great! Thank you
There was a problem hiding this comment.
:D the fork was just there to speed up the development and the integration of this lib into Prometheus and PromLens.
It was also a good way to test if cmn is working properly with another implementation of the autocompletion module.
But even if CMN is working as a module, it is still simpler to use the vanilla modules. It simplify so many things !
There was a problem hiding this comment.
and you are really welcome :)
| apply: snippet(s.snippet), | ||
| score: 0, |
There was a problem hiding this comment.
Please don't think this is a review, I just have a question. After this PR we still don't have scoring in this hybrid mode, right? I mean we haven't used CM's boost option yet?
There was a problem hiding this comment.
actually the scoring is fully integrated in the autocomplete module. The boost is just a field to say "this particular label is really important". It's a way to influence a bit the internal scoring system.
For the moment I don't see with what condition we should use it, but you can be sure the list returned is using an internal score.
There was a problem hiding this comment.
oh thank you. I've just went and check the autocomplete module and I saw the internal scoring system 👍
| snippet: 'histogram_quantile(${__quantile__}, sum by(le) (rate(${__histogram_metric__}[5m])))', | ||
| }, | ||
| { | ||
| keyword: 'label_replace(__input_vector__, "__dst__", "__replacement__", "__src__", "__regex__")', | ||
| label: 'label_replace(__input_vector__, "__dst__", "__replacement__", "__src__", "__regex__")', | ||
| type: 'function', |
There was a problem hiding this comment.
It seems this type is not propagated into the parsedSnippets and thus not used anywhere? Not sure it should be function though. Should we invent our own snippet type?
There was a problem hiding this comment.
And then if it's always snippet for snippets, I guess we can just hardcode it in the parsedSnippets below.
There was a problem hiding this comment.
It would be nice indeed to a have a dedicated icon for the snippet !
I will review the snippet management it seems I was a bit too fast here
There was a problem hiding this comment.
And then if it's always snippet for snippets,
What do you mean by that ? Not sure I got your point actually :(.
There was a problem hiding this comment.
I simplify the way to manage the snippet, hope you thought it like that too !
There was a problem hiding this comment.
Ah ok yeah, what you have now is fine too.
|
👍 Super nice, thank you :) Btw., the snippet selection by mouse now works for me, but the editor is unfocussed afterwards. |
|
A bit weird the snippet is working since the patch is not yet released :o |
|
👍 |
src/lang-promql/complete/hybrid.ts
Outdated
| for (const s of parsedSnippets) { | ||
| options.push(s); | ||
| } | ||
| options.concat(snippets); |
There was a problem hiding this comment.
@Nexucis Is this intentional? I think concat doesn't change the options array at all.
If you don't like the for loop then I think you can still do options.push(...snippets) if snippets should be included in options
There was a problem hiding this comment.
Ah yeah I forgot to get back the array returned. Thanks !
There was a problem hiding this comment.
And yeah it's maybe better to follow your way. Should cost less to keep the array and to push the element.
Thanks for the suggestion
There was a problem hiding this comment.
it's fixed. Thanks for the review @dvkndn :)
Signed-off-by: Augustin Husson <husson.augustin@gmail.com>
Nexucis
force-pushed
the
feature/autocomplete-v2
branch
from
ad2a7af
to
fab5dc9
Compare
Should fix prometheus#14, fix prometheus#25 and let the help in prometheus#50 be shown
fix #47
should partially solved some issue raised by #48 as well