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.

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

Syntax highlight in hover #1336

Merged
merged 14 commits into from
May 21, 2024
Merged

Syntax highlight in hover #1336

merged 14 commits into from
May 21, 2024

Conversation

Wck-iipi
Copy link
Contributor

@Wck-iipi Wck-iipi commented May 20, 2024

1. Does this PR affect any open issues?(Y/N) and add issue references (e.g. "fix #123", "re #123".):

2. What is the scope of this PR (e.g. component or file name):

/tools/src/LSP/src/hover.rs

3. Provide a description of the PR(e.g. more details, effects, motivations or doc link):

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory
  • Other
    It provides syntax highlight to hover text. It does so by using MarkedString::LanguageString and MarkedString::String.

This fixes the formatting as seen below.
image

However, this has a problem. It doesn't work on neovim unless you set the language as "kwt" instead of kcl. Therefore, please check if this is case for you as well.
image

4. Are there any breaking changes?(Y/N) and describe the breaking changes(e.g. more details, motivations or doc link):

  • N
  • Y

5. Are there test cases for these changes?(Y/N) select and add more details, references or doc links:

  • Unit test
  • Integration test
  • Benchmark (add benchmark stats below)
  • Manual test (add detailed scripts or steps below)
  • Other

I have currently not written tests for this. Please tell me if this is the right solution and then I will write the test cases.

Signed-off-by: Wck-iipi <110763795+Wck-iipi@users.noreply.github.com>
Signed-off-by: Wck-iipi <110763795+Wck-iipi@users.noreply.github.com>
Copy link

github-actions bot commented May 20, 2024

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@Peefy
Copy link
Contributor

Peefy commented May 20, 2024

LGTM. Thanks for the contribution. Please sign the CLA, fix the test cases in CI and I will merge this PR.

@Wck-iipi
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

Peefy added a commit to kcl-lang/cla.db that referenced this pull request May 20, 2024
@Wck-iipi
Copy link
Contributor Author

@Peefy I was writing test for above code but I realised that I have to change tests for decorators, functions. But the problem with this is that the classifications I have done for these(except schema and attributes) don't cover all the cases. This will result in me writing some make-do tests for these and the person working after me will have to do extra work of removing those test-cases and then writing the correct code. Therefore, I was thinking if I should get the correct highlight for all data-types mentioned(Value and Decorators) and then writing test cases for the correct ones. Or should I just stick to original work.

@Peefy
Copy link
Contributor

Peefy commented May 21, 2024

@Peefy I was writing test for above code but I realised that I have to change tests for decorators, functions. But the problem with this is that the classifications I have done for these(except schema and attributes) don't cover all the cases. This will result in me writing some make-do tests for these and the person working after me will have to do extra work of removing those test-cases and then writing the correct code. Therefore, I was thinking if I should get the correct highlight for all data-types mentioned(Value and Decorators) and then writing test cases for the correct ones. Or should I just stick to original work.

If you could add the correct display and testing for all hover content, that would be great. Unable to merge the PR in case of testing errors.

kclvm/tools/src/LSP/src/hover.rs Outdated Show resolved Hide resolved
kclvm/tools/src/LSP/src/hover.rs Outdated Show resolved Hide resolved
Signed-off-by: Wck-iipi <110763795+Wck-iipi@users.noreply.github.com>
Signed-off-by: Wck-iipi <110763795+Wck-iipi@users.noreply.github.com>
Signed-off-by: Wck-iipi <110763795+Wck-iipi@users.noreply.github.com>
Signed-off-by: Wck-iipi <110763795+Wck-iipi@users.noreply.github.com>
@Wck-iipi
Copy link
Contributor Author

@Peefy @He1pa I have added hover for all data types. Please check if this is correct formatting. I am currently writing tests for these.
image
image
image

@Wck-iipi Wck-iipi requested a review from He1pa May 21, 2024 10:52
Signed-off-by: Wck-iipi <110763795+Wck-iipi@users.noreply.github.com>
Signed-off-by: Wck-iipi <110763795+Wck-iipi@users.noreply.github.com>
Signed-off-by: Wck-iipi <110763795+Wck-iipi@users.noreply.github.com>
Signed-off-by: Wck-iipi <110763795+Wck-iipi@users.noreply.github.com>
Signed-off-by: Wck-iipi <110763795+Wck-iipi@users.noreply.github.com>
@Wck-iipi
Copy link
Contributor Author

Wck-iipi commented May 21, 2024

I have also written the required tests for all. Anything else to be done?

No.

If all tests pass in CI. I'll merge this PR.

@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 9174095710

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 101 of 107 (94.39%) changed or added relevant lines in 3 files are covered.
  • 584 unchanged lines in 15 files lost coverage.
  • Overall coverage increased (+0.03%) to 71.087%

Changes Missing Coverage Covered Lines Changed/Added Lines %
kclvm/sema/src/ty/mod.rs 22 24 91.67%
kclvm/tools/src/LSP/src/hover.rs 47 51 92.16%
Files with Coverage Reduction New Missed Lines %
kclvm/parser/src/parser/stmt.rs 2 95.28%
kclvm/query/src/lib.rs 2 92.86%
kclvm/runtime/src/value/api.rs 2 0.31%
kclvm/evaluator/src/context.rs 3 81.46%
kclvm/parser/src/parser/module.rs 3 93.48%
kclvm/evaluator/src/proxy.rs 4 0.0%
kclvm/ast/src/walker.rs 8 42.06%
kclvm/runtime/src/api/kclvm.rs 9 44.53%
kclvm/parser/src/session/mod.rs 10 77.19%
kclvm/tools/src/LSP/src/hover.rs 11 89.89%
Totals Coverage Status
Change from base Build 9156283670: 0.03%
Covered Lines: 53829
Relevant Lines: 75723

💛 - Coveralls

Signed-off-by: Wck-iipi <110763795+Wck-iipi@users.noreply.github.com>
@Wck-iipi
Copy link
Contributor Author

@Peefy All tests except hover::tests::dict_key_in_schema(which was added an hour ago) are passing in previous run. Should I change this test as well?

@Peefy
Copy link
Contributor

Peefy commented May 21, 2024

@Peefy All tests except hover::tests::dict_key_in_schema(which was added an hour ago) are passing in previous run. Should I change this test as well?

Yes, thank you! 😄

Signed-off-by: Wck-iipi <110763795+Wck-iipi@users.noreply.github.com>
@Wck-iipi
Copy link
Contributor Author

Done @Peefy

@Peefy Peefy merged commit 6bf352c into kcl-lang:main May 21, 2024
7 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants