Skip to content

Fix missing unit in Gauge metrics snippet#47

Merged
angelozerr merged 1 commit intoeclipse-lsp4mp:masterfrom
rzgry:fix-metric-snippet
Sep 3, 2020
Merged

Fix missing unit in Gauge metrics snippet#47
angelozerr merged 1 commit intoeclipse-lsp4mp:masterfrom
rzgry:fix-metric-snippet

Conversation

@rzgry
Copy link
Copy Markdown
Contributor

@rzgry rzgry commented Aug 10, 2020

@fbricon
Copy link
Copy Markdown
Contributor

fbricon commented Aug 19, 2020

"Fix missing comma"?

@rzgry
Copy link
Copy Markdown
Contributor Author

rzgry commented Aug 19, 2020

This PR adds the unit attribute in the @Gauge annotation snippet since it was missing: "\tunit = $3",. The added comma in the first line is needed just to add the new attribute.

@fbricon fbricon requested a review from angelozerr August 28, 2020 13:12
"\tname = \"${1:name}\",",
"\tdescription = \"${2:description}\"",
"\tdescription = \"${2:description}\",",
"\tunit = $3",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unit is a string, so I think it should be

"\tunit = \"${3:unit}\"",

no?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think for the majority of use cases the MetricUnit constants are used. https://github.com/eclipse/microprofile-metrics/blob/master/api/src/main/java/org/eclipse/microprofile/metrics/MetricUnits.java#L34

Ex. from https://quarkus.io/guides/microprofile-metrics
@Gauge(name = "highestPrimeNumberSoFar", unit = MetricUnits.NONE, description = "Highest prime number so far.")

So i didn't want to wrap the placeholder in quotes if they would need to remove them to use the MetricUnit constants.

I would think we leave it as is, or maybe change it to something like: "\tunit = ${3:\"unit\"}",?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Snippets should generate code without syntax error, that's why I wonder why it misses quote, but I understand your point of view. So I suggest that you use snippet choice https://microsoft.github.io/language-server-protocol/specification#choice which provides MetricUnits.NONE, and other value of MetricUnits and double quote.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to use the choice syntax. I only added a couple options. Not sure if we should cover all possible MetricUnits
options? Seems like it might be too much.

image

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's nice now because snippets generates annotation without syntax error. Thanks @rzgry !

@rzgry rzgry force-pushed the fix-metric-snippet branch from 299e866 to e28a774 Compare September 2, 2020 18:13
@angelozerr angelozerr added this to the 0.9.0 milestone Sep 3, 2020
@angelozerr angelozerr merged commit fb49ce9 into eclipse-lsp4mp:master Sep 3, 2020
@angelozerr angelozerr added the bug Something isn't working label Sep 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working snippets

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants