Allow formatting PromQL expressions in the UI

[juliusv]

Signed-off-by: Julius Volz [email protected]

Note 1: To be 100% correct & safe I would have to also make the text editor read-only (and visually clearly so) while a format request is in-flight, but I thought that would be over-engineering it for a request that should normally finish instantaneously.

Note 2: In the video below, I introduced an artificial 1-second delay for the request so that it's possible to see the loading behavior:

test-2022-07-20_14.47.11.mp4

[juliusv]

@Harkishen-Singh @codesome

[codesome]

Amazing! No idea about the react code. And not strong opinion on the button icon. Can it be something like PP which means pretty print?

[roidelapluie]

I like the icon selected by Julius.

[juliusv]

I like the icon selected by Julius.

Most of the time went into deciding that part, so I appreciate that 😆

[juliusv]

Can it be something like PP which means pretty print?

I haven't seen such an icon around yet, but let me know if you see a better one. We could of course theoretically also create our own, but that seems like a disproportionate amount of effort. However, not sure if I like "PP". That is also not understandable unless you hover over it.

[codesome]

Let's go with the current icon :)

[Nexucis]

@juliusv would be cool to block the button when the codemirror linter returns some errors

[juliusv]

Great!

I pushed one more change to the fetch() error handling so that HTTP errors are handled properly too, in addition to the API-level errors.

[juliusv]

@juliusv would be cool to block the button when the codemirror linter returns some errors

It would be, kinda. Is it worth it the extra complexity though to get that state from CodeMirror (not sure how to do it)? And from the user's perspective, if the button is blocked I would need to explain to the user why (maybe change the title property). If it is like it's now, they will at least know due to the large error display why the formatting isn't working.

[roidelapluie]

It would be great to add a green check when the query is formatted already (when we press the format button, until the input is changed again). because for now I find it confusing that it "apparently does nothing" when the query is already formatted (e.g when you click multiple times).

[juliusv]

@roidelapluie Added that (although for now I didn't make it green, maybe keeping it more low-key for such a functionality is better?):

test-2022-07-20_16.16.34.mp4

I am also changing the title texts now depending on the situation.

[Nexucis]

It would be, kinda. Is it worth it the extra complexity though to get that state from CodeMirror (not sure how to do it)?

you can do it by using the method diagnosticCount. If it is more than 0 then you the button can be blocked.

if the button is blocked I would need to explain to the user why (maybe change the title property)

Totally yes. The message can be simply : "unable to format an incorrect expression"

[Nexucis]

an advantage to block the button if the expression is not valid would be to not handle the http error. Based on the fact if codemirror doesn't yell at you, then your expression is correct and the server will be ok

[juliusv]

you can do it by using the method diagnosticCount. If it is more than 0 then you the button can be blocked.

Thanks! I managed to do that now, but it's a bit laggy because the linter only responds within a few milliseconds (the linter update is not yet available in the same updateListener callback call as the actual docs changes that precede it).

an advantage to block the button if the expression is not valid would be to not handle the http error.

IMO we should still always check those errors. There are other reasons why an API call may fail other than an invalid parameter, and who knows if the client-side checks really cover 100% of what we have in Go :)

[Nexucis]

so if you format the expression, then the doc changed so exprFormatted is equal to false ?

[Nexucis]

also you could simply do

onExpressionChange(update.state.doc.toString());
setExprFormatted(false);

[juliusv]

Yes, the only (weak) reason for the if condition is to not cause yet another React state update when it's not needed (if exprFormatted is already false, we don't need to set it to false), since this happens on every typed character. But yeah, probably it doesn't matter at all. I also tried to do the same with the setHasLinterErrors(), but it led to weird feedback issues due to the lag, so now I'm just always setting it.

[Nexucis]

@juliusv I tested your branch locally and it seems there is an issue with the event. It appears that once it is formatted, the icon doesn't come back to the original state when you change the expression.

Screen.Recording.2022-07-21.at.14.32.43.mov

[Nexucis]

I also just realised that the button Execute that runs the query is not blocked when the codemirror linter is showing some errors.

So I would say to be consistent: We blocked this button as well when it displayed an error, or we don't block it.

By looking at the number of states of the format button, I would tend in favor of not blocking the button (even if I was proposing it initially).

[juliusv]

@Nexucis Ah drat, I broke that by wrapping the if (exprFormatted) around the setExprFormatted(false). I think it's because the surrounding function is a stale closure wrapping the exprFormatted value from the first render of the overall React component, so it always is false.

So I can easily fix it by just removing the if condition and I could easily disable the execute button as well. But yeah, maybe it's better in the interest of simplicity to not block these buttons depending on linter state. So I remove that, yeah?

[Nexucis]

yeah let's remove it @juliusv, it will be simpler and it's not that a huge issue that you tried to format a wrong expression at the end. It would be coherent to block these buttons, but I thought it would be simpler and it is not. So I'm more in favor in removing it now :)

[juliusv]

@roidelapluie Yup, removed it again!

[juliusv]

Eh sorry, meant to tag @Nexucis.

[Nexucis]

nice, thanks @juliusv :)