Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix: Scalar parameter docstring and example arguments unused #479

Merged
merged 21 commits into from
Oct 7, 2024
Merged

fix: Scalar parameter docstring and example arguments unused #479

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
947d3be
fix: Parameter docstring and example arguments unused
maxburs Aug 22, 2024
d96e515
Merge branch 'master' of https://github.com/Azure/monaco-kusto into u…
maxburs Sep 27, 2024
a3c3691
Added tests
maxburs Sep 27, 2024
573a0b6
Bump package version
maxburs Sep 27, 2024
fc9eeb2
Fixed failing test
maxburs Sep 28, 2024
3660229
Increase timeout
maxburs Oct 2, 2024
a47dbd8
Upload playwright report
maxburs Oct 2, 2024
5e4eaa6
Where's my report?
maxburs Oct 2, 2024
e9769d0
Ok, thats why
maxburs Oct 2, 2024
84f12f5
Retain more traces
maxburs Oct 2, 2024
ce5c360
Woops
maxburs Oct 2, 2024
1959be9
Remove snapshots
maxburs Oct 3, 2024
76b990a
Likely wont work
maxburs Oct 3, 2024
2da5447
Try something else
maxburs Oct 3, 2024
9e84799
Cleanup diff and try adding timeout
maxburs Oct 3, 2024
e2e83e7
Ugggg
maxburs Oct 3, 2024
9fc9afa
Revert
maxburs Oct 3, 2024
41ec976
Tweak
maxburs Oct 3, 2024
b4025eb
Try revert
maxburs Oct 3, 2024
fc684e3
Cleanup
maxburs Oct 3, 2024
43bc6f3
Revert more stuff
maxburs Oct 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,10 @@ Every PR should come with a test that checks it.

### 12.0.5

- fix: Parameter docstrings are now shown on hover

### 12.0.5

- fix: Dark mode colors are now more readable.

### 12.0.4
Expand Down
2 changes: 1 addition & 1 deletion package/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@kusto/monaco-kusto",
"version": "12.0.5",
"version": "12.0.6",
"description": "CSL, KQL plugin for the Monaco Editor",
"author": {
"name": "Microsoft"
Expand Down
6 changes: 3 additions & 3 deletions package/src/languageServiceManager/kustoLanguageService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1695,7 +1695,7 @@ class KustoLanguageService implements LanguageService {
const paramSymbol: sym.ScalarSymbol = Kusto.Language.Symbols.ScalarTypes.GetSymbol(
getCslTypeNameFromClrType(param.type)
);
return new sym.ParameterSymbol(param.name, paramSymbol, null);
return new sym.ParameterSymbol(param.name, paramSymbol, param.docstring ?? null);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tested this change, others are a bit more speculative

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not clear on the use case.
Could we add a unit or integration test to clarify it and help prevent regressions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Examples and docstring/description are added to tooltips on hover. I'm using them for dashboard system parameters, and and I'd assume other ones are in the system we get from kusto (otherwise, IDK why the C# api would expose it)

Could we add a unit or integration test to clarify it and help prevent regressions?

Unlike a lot of other stuff, this isn't really load bearing. If it breaks it won't block a release. I don't think it should be a priority for testing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If a unit test would help I might add that, because it would be easy, but I don't think it would do anything useful.

An integration tests would be pretty expensive to add.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm sorry if I wasn't clear.
From my perspective, testing is an important part of our work as engineers.
It helps ensure that what we build remains reliable and understandable for other developers.
I would suggest adding an integration test here, but if that's too difficult, a unit test would also be fine.

Copy link
Contributor

@sagivf sagivf Aug 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@maxburs can you add some examples/screenshots of where this shows up and how?
That would help in understanding how/what test we should add.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

}

private static createTabularParameterSymbol(param: s.TabularParameter): sym.ParameterSymbol {
Expand All @@ -1720,13 +1720,13 @@ class KustoLanguageService implements LanguageService {
paramSymbol,
null,
null,
null,
param.examples ? KustoLanguageService.toBridgeList(param.examples) : null,
false,
null,
1,
1,
expression,
null
param.docstring ?? null
);
}

Expand Down
Loading