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

feat: lsp hover keyword highlight #1331

Merged
merged 11 commits into from
May 22, 2024
Merged

Conversation

shruti2522
Copy link
Contributor

@shruti2522 shruti2522 commented May 17, 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):

kcl/kclvm/tools/src/LSP/src/hover.rs
kcl/kclvm/sema/src/ty/mod.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

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

cc @He1pa @Peefy

@shruti2522 shruti2522 force-pushed the lsp-keyword-highlight branch 2 times, most recently from d360af1 to 182f217 Compare May 17, 2024 20:43
@shruti2522
Copy link
Contributor Author

shruti2522 commented May 17, 2024

I have added Atrributes to the keywords in KCL.tmLanguage.json and this is how the changes look locally @Peefy @He1pa

Screenshot from 2024-05-17 22-59-40

@coveralls
Copy link
Collaborator

coveralls commented May 17, 2024

Pull Request Test Coverage Report for Build 9188427383

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

  • 33 of 33 (100.0%) changed or added relevant lines in 4 files are covered.
  • 23 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.03%) to 71.062%

Files with Coverage Reduction New Missed Lines %
kclvm/tools/src/LSP/src/hover.rs 1 90.0%
kclvm/sema/src/ty/mod.rs 22 77.32%
Totals Coverage Status
Change from base Build 9177105510: -0.03%
Covered Lines: 53813
Relevant Lines: 75727

💛 - Coveralls

@shruti2522 shruti2522 marked this pull request as ready for review May 17, 2024 22:27
@shruti2522 shruti2522 marked this pull request as draft May 18, 2024 09:07
@shruti2522 shruti2522 changed the title feat: lsp keyword highlight feat: lsp hover keyword highlight May 19, 2024
@shruti2522
Copy link
Contributor Author

shruti2522 commented May 19, 2024

The current hover looks like this, is this the required keyword syntax highlighting? @Peefy @He1pa

Screenshot from 2024-05-19 22-57-46

Screenshot from 2024-05-19 22-58-05

@Peefy
Copy link
Contributor

Peefy commented May 20, 2024

I have added Atrributes to the keywords in KCL.tmLanguage.json and this is how the changes look locally @Peefy @He1pa

Screenshot from 2024-05-17 22-59-40

Hello @shruti2522 The hover content expect a right kcl code form with indentation like the rust-analyzer does.
image

Furthermore, I do not believe that adding ````kcl` is the correct approach for two reasons

  1. You cannot ensure that all IDE clients such as VS Code, idea render hovers in a markdown format, right?
  2. kcl is not a label that can be marked down for normal rendering in the markdown files.

cc @He1pa

@shruti2522 shruti2522 force-pushed the lsp-keyword-highlight branch 3 times, most recently from 5830195 to 2446911 Compare May 20, 2024 16:43
@shruti2522 shruti2522 marked this pull request as ready for review May 20, 2024 18:00
@shruti2522
Copy link
Contributor Author

shruti2522 commented May 20, 2024

PTAL @Peefy @He1pa

@shruti2522
Copy link
Contributor Author

current hover:

Screenshot from 2024-05-20 23-31-21

@He1pa
Copy link
Contributor

He1pa commented May 21, 2024

The current hover looks like this, is this the required keyword syntax highlighting? @Peefy @He1pa

You dont need to follow the previous stlpe, and the Attributes is not must required
The previous hover content was:

pkg
schema Name
---
doc
---
Attributes:
attr1: type1
attr2: type2

Now can be rendered as

pkg
```kcl
schema Name:
attr1: type1
attr2: type2

doc


Or some other better style.

@shruti2522
Copy link
Contributor Author

updated doc style @He1pa

Screenshot from 2024-05-21 10-44-42

@Peefy Peefy requested a review from He1pa May 21, 2024 05:29
@shruti2522
Copy link
Contributor Author

added indentation for attrs. working on updating the test cases for it.

Screenshot from 2024-05-21 13-24-23

@Peefy
Copy link
Contributor

Peefy commented May 21, 2024

@shruti2522
Good Job! Besides I suggest merging the hovering market string of schema name and schema attribute into one (merge docs[0] and docs[1] to docs[0]).

@shruti2522
Copy link
Contributor Author

Done @Peefy

Screenshot from 2024-05-21 14-02-48

@Peefy
Copy link
Contributor

Peefy commented May 21, 2024

Done @Peefy

Screenshot from 2024-05-21 14-02-48

According to the review comments, what I mean is to merge the content of the schema doc hover instead of merging the content of the docs to hover.

@shruti2522
Copy link
Contributor Author

shruti2522 commented May 21, 2024

According to the review comments, what I mean is to merge the content of the schema doc hover instead of merging the content of the docs to hover.

If I understand correctly, what I need to do is to merge all the contents of docs into one such that the complete hover content is displayed as one without any separating lines? @Peefy

@Peefy
Copy link
Contributor

Peefy commented May 21, 2024

According to the review comments, what I mean is to merge the content of the schema doc hover instead of merging the content of the docs to hover.

If I understand correctly, what I need to do is to merge all the contents of docs into one such that the complete hover content is displayed as one without any separating lines? @Peefy

Just merge the schema doc instead of merge all docs in docs_to_hover function.

@shruti2522
Copy link
Contributor Author

Hey @Peefy @He1pa , I have updated all the newly added test cases. Any further changes?

Signed-off-by: shruti2522 <[email protected]>

fmt check

Signed-off-by: shruti2522 <[email protected]>

kcl formatting

Signed-off-by: shruti2522 <[email protected]>

fmt check

Signed-off-by: shruti2522 <[email protected]>

updated test cases

Signed-off-by: shruti2522 <[email protected]>

fmt check

Signed-off-by: shruti2522 <[email protected]>

feat: syntax highlighting for hover content

Signed-off-by: shruti2522 <[email protected]>

updated tests

Signed-off-by: shruti2522 <[email protected]>

updated tests

Signed-off-by: shruti2522 <[email protected]>

test check

Signed-off-by: shruti2522 <[email protected]>

feat: syntax highlighting for hover content

Signed-off-by: shruti2522 <[email protected]>

add markup

Signed-off-by: shruti2522 <[email protected]>

fmt check

Signed-off-by: shruti2522 <[email protected]>

resolved func formatting error

Signed-off-by: shruti2522 <[email protected]>

updated test cases

Signed-off-by: shruti2522 <[email protected]>

updated tests

Signed-off-by: shruti2522 <[email protected]>

feat: syntax highlighting for hover content

Signed-off-by: shruti2522 <[email protected]>

updated test cases for tests.rs

Signed-off-by: shruti2522 <[email protected]>

feat: syntax highlighting for hover content

Signed-off-by: shruti2522 <[email protected]>

syntax highlighting with LanguageString

Signed-off-by: shruti2522 <[email protected]>

fmt check

Signed-off-by: shruti2522 <[email protected]>

updated tests

Signed-off-by: shruti2522 <[email protected]>

fmt check

Signed-off-by: shruti2522 <[email protected]>

fmt check

Signed-off-by: shruti2522 <[email protected]>

feat: syntax highlighting for hover content

Signed-off-by: shruti2522 <[email protected]>

updated test case

Signed-off-by: shruti2522 <[email protected]>

updated tests.rs

Signed-off-by: shruti2522 <[email protected]>

feat: syntax highlighting for hover content

Signed-off-by: shruti2522 <[email protected]>

fmt check

Signed-off-by: shruti2522 <[email protected]>

fixed ci

Signed-off-by: shruti2522 <[email protected]>

add pkgpath without override

Signed-off-by: shruti2522 <[email protected]>

updated completion.rs

Signed-off-by: shruti2522 <[email protected]>

updated tests.rs

Signed-off-by: shruti2522 <[email protected]>

feat: syntax highlighting for hover content

Signed-off-by: shruti2522 <[email protected]>

updated doc style

Signed-off-by: shruti2522 <[email protected]>

fix ci

Signed-off-by: shruti2522 <[email protected]>

updated comment

Signed-off-by: shruti2522 <[email protected]>

added indentation for attrs

Signed-off-by: shruti2522 <[email protected]>

fix ci

Signed-off-by: shruti2522 <[email protected]>

merge schema attr docs

Signed-off-by: shruti2522 <[email protected]>

feat: syntax highlighting for hover content

Signed-off-by: shruti2522 <[email protected]>

update hover def comment

Signed-off-by: shruti2522 <[email protected]>

updated tests

Signed-off-by: shruti2522 <[email protected]>

feat: syntax highlighting for hover content

Signed-off-by: shruti2522 <[email protected]>

fix ci

Signed-off-by: shruti2522 <[email protected]>

render doc at end

Signed-off-by: shruti2522 <[email protected]>

fix ci

Signed-off-by: shruti2522 <[email protected]>

removed additional def

Signed-off-by: shruti2522 <[email protected]>

feat: syntax highlighting for hover content

Signed-off-by: shruti2522 <[email protected]>

fix ci

Signed-off-by: shruti2522 <[email protected]>

fix dict_key_in_schema test

Signed-off-by: shruti2522 <[email protected]>

merged rest_sign and attr

Signed-off-by: shruti2522 <[email protected]>

fixed ci

Signed-off-by: shruti2522 <[email protected]>

fmt check

Signed-off-by: shruti2522 <[email protected]>

feat: syntax highlighting for hover content

Signed-off-by: shruti2522 <[email protected]>
@shruti2522
Copy link
Contributor Author

Done with the changes @Peefy

@Peefy
Copy link
Contributor

Peefy commented May 21, 2024

PR conflict

shruti2522 and others added 3 commits May 21, 2024 22:10
Signed-off-by: shruti2522 <[email protected]>

fmt check

Signed-off-by: shruti2522 <[email protected]>

kcl formatting

Signed-off-by: shruti2522 <[email protected]>

fmt check

Signed-off-by: shruti2522 <[email protected]>

updated test cases

Signed-off-by: shruti2522 <[email protected]>

fmt check

Signed-off-by: shruti2522 <[email protected]>

feat: syntax highlighting for hover content

Signed-off-by: shruti2522 <[email protected]>

updated tests

Signed-off-by: shruti2522 <[email protected]>

updated tests

Signed-off-by: shruti2522 <[email protected]>

test check

Signed-off-by: shruti2522 <[email protected]>

feat: syntax highlighting for hover content

Signed-off-by: shruti2522 <[email protected]>

add markup

Signed-off-by: shruti2522 <[email protected]>

fmt check

Signed-off-by: shruti2522 <[email protected]>

resolved func formatting error

Signed-off-by: shruti2522 <[email protected]>

updated test cases

Signed-off-by: shruti2522 <[email protected]>

updated tests

Signed-off-by: shruti2522 <[email protected]>

feat: syntax highlighting for hover content

Signed-off-by: shruti2522 <[email protected]>

updated test cases for tests.rs

Signed-off-by: shruti2522 <[email protected]>

feat: syntax highlighting for hover content

Signed-off-by: shruti2522 <[email protected]>

syntax highlighting with LanguageString

Signed-off-by: shruti2522 <[email protected]>

fmt check

Signed-off-by: shruti2522 <[email protected]>

updated tests

Signed-off-by: shruti2522 <[email protected]>

fmt check

Signed-off-by: shruti2522 <[email protected]>

fmt check

Signed-off-by: shruti2522 <[email protected]>

feat: syntax highlighting for hover content

Signed-off-by: shruti2522 <[email protected]>

updated test case

Signed-off-by: shruti2522 <[email protected]>

updated tests.rs

Signed-off-by: shruti2522 <[email protected]>

feat: syntax highlighting for hover content

Signed-off-by: shruti2522 <[email protected]>

fmt check

Signed-off-by: shruti2522 <[email protected]>

fixed ci

Signed-off-by: shruti2522 <[email protected]>

add pkgpath without override

Signed-off-by: shruti2522 <[email protected]>

updated completion.rs

Signed-off-by: shruti2522 <[email protected]>

updated tests.rs

Signed-off-by: shruti2522 <[email protected]>

feat: syntax highlighting for hover content

Signed-off-by: shruti2522 <[email protected]>

updated doc style

Signed-off-by: shruti2522 <[email protected]>

fix ci

Signed-off-by: shruti2522 <[email protected]>

updated comment

Signed-off-by: shruti2522 <[email protected]>

added indentation for attrs

Signed-off-by: shruti2522 <[email protected]>

fix ci

Signed-off-by: shruti2522 <[email protected]>

merge schema attr docs

Signed-off-by: shruti2522 <[email protected]>

feat: syntax highlighting for hover content

Signed-off-by: shruti2522 <[email protected]>

update hover def comment

Signed-off-by: shruti2522 <[email protected]>

updated tests

Signed-off-by: shruti2522 <[email protected]>

feat: syntax highlighting for hover content

Signed-off-by: shruti2522 <[email protected]>

fix ci

Signed-off-by: shruti2522 <[email protected]>

render doc at end

Signed-off-by: shruti2522 <[email protected]>

fix ci

Signed-off-by: shruti2522 <[email protected]>

removed additional def

Signed-off-by: shruti2522 <[email protected]>

feat: syntax highlighting for hover content

Signed-off-by: shruti2522 <[email protected]>

fix ci

Signed-off-by: shruti2522 <[email protected]>

fix dict_key_in_schema test

Signed-off-by: shruti2522 <[email protected]>

merged rest_sign and attr

Signed-off-by: shruti2522 <[email protected]>

fixed ci

Signed-off-by: shruti2522 <[email protected]>

fmt check

Signed-off-by: shruti2522 <[email protected]>

feat: syntax highlighting for hover content

Signed-off-by: shruti2522 <[email protected]>
Signed-off-by: shruti2522 <[email protected]>
@shruti2522
Copy link
Contributor Author

shruti2522 commented May 21, 2024

Resolved conflicts @Peefy

kclvm/tools/src/LSP/src/hover.rs Outdated Show resolved Hide resolved
kclvm/tools/src/LSP/src/hover.rs Outdated Show resolved Hide resolved
kclvm/tools/src/LSP/src/hover.rs Outdated Show resolved Hide resolved
kclvm/tools/src/LSP/src/hover.rs Outdated Show resolved Hide resolved
@Peefy
Copy link
Contributor

Peefy commented May 22, 2024

Hello @shruti2522 Add some comments.

Signed-off-by: shruti2522 <[email protected]>
@shruti2522
Copy link
Contributor Author

schema hover:

Screenshot from 2024-05-22 11-38-03

fn hover:

Screenshot from 2024-05-22 11-43-37

type hover:

Screenshot from 2024-05-22 11-42-15

cc @Peefy @He1pa

kclvm/tools/src/LSP/src/hover.rs Outdated Show resolved Hide resolved
kclvm/tools/src/LSP/src/hover.rs Outdated Show resolved Hide resolved
@Peefy
Copy link
Contributor

Peefy commented May 22, 2024

Hello @shruti2522 Add some comments and CI failed.

Signed-off-by: shruti2522 <[email protected]>
@shruti2522
Copy link
Contributor Author

shruti2522 commented May 22, 2024

CI shows:

assertion `left == right` failed
  left: [LanguageString(LanguageString { language: "KCL", value: "schedulingStrategy: SchedulingStrategy" }), String("SchedulingStrategy represents scheduling strategy.")]
 right: [LanguageString(LanguageString { language: "KCL", value: "schedulingStrategy: SchedulingStrategy" }), LanguageString(LanguageString { language: "KCL", value: "SchedulingStrategy represents scheduling strategy." })]

but the expected value in konfig_hover_test_main is :

let expect: Vec<MarkedString> = vec![
                MarkedString::LanguageString(lsp_types::LanguageString {
                    language: "KCL".to_string(),
                    value: "schedulingStrategy: SchedulingStrategy".to_string(),
                }),
                MarkedString::String(
                    "SchedulingStrategy represents scheduling strategy.".to_string(),
                ),
            ];

@Peefy

@Peefy
Copy link
Contributor

Peefy commented May 22, 2024

This indicates that there is an issue with the implementation logic of the hover in this PR. Please carefully check the code. @shruti2522

Signed-off-by: shruti2522 <[email protected]>
@shruti2522
Copy link
Contributor Author

All checks have passed @Peefy

kclvm/tools/src/LSP/src/hover.rs Outdated Show resolved Hide resolved
kclvm/tools/src/LSP/src/hover.rs Outdated Show resolved Hide resolved
@Peefy
Copy link
Contributor

Peefy commented May 22, 2024

@shruti2522

Although all the test cases have been passed, I believe there is an issue with the implementation of docs_to_hover, which resulted in incorrect test cases. Please refer to the review comments for modifications.

cc @He1pa Could you please check it again?

Signed-off-by: shruti2522 <[email protected]>
@shruti2522
Copy link
Contributor Author

I have updated the test cases in hover to use LanguageString in place of String wherever required and the test cases pass locally. @Peefy

Signed-off-by: shruti2522 <[email protected]>
Signed-off-by: shruti2522 <[email protected]>
Copy link
Contributor

@Peefy Peefy left a comment

Choose a reason for hiding this comment

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

LGTM

@Peefy Peefy merged commit dac7abf into kcl-lang:main May 22, 2024
8 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.

[LFX PreTest] pretest for LFX LSP hover content
4 participants