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

misc #35

Closed
wants to merge 20 commits into from
Closed

misc #35

wants to merge 20 commits into from

Conversation

git-bruh
Copy link
Contributor

@git-bruh git-bruh commented Apr 6, 2024

Hey, I've been maintaining a fork of vis-lspc to adapt it to my needs, just dropping this PR here so you can cherry pick any commits you might find useful. Most commits are atomic, some changes:

  • Fetch the line's content from the file (either by re-using the existing vis buffer or opening it via the general lua APIs)
  • Convert paths to relative paths so more content is visible when being piped to a program like fzy
  • Hover messages are requested in markdown
  • Different styling for each type of diagnostic message
  • vis:pipe refactors to use vis:pipe_buffer, as I noticed the printf command construction breaking in large codebases

If you'd be interested in having some of this upstream I can create individual PRs

Thanks a lot for creating this project!

@fischerling
Copy link
Owner

Thank very much for your contribution!
I will look at your changes when I have time and reach out to you if necessary.

@fischerling
Copy link
Owner

fischerling commented Apr 10, 2024

Applyied so far: (sorry for making this harder to rebase)

TODO

  • relative paths
  • show messages in new windows using syntax highlghting
  • show line content
  • detect and propagate project roots

return 'c'
end
return syntax
local map = {
Copy link
Owner

Choose a reason for hiding this comment

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

This does not seem correct to me. What is the reason for this?

List of language IDs

  • JavaScript uses javascript not jsx
  • TypeScript uses typescript not tsx

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah it's a workaround for jsx/tsx files which are a superset of javascript/typescript allowing for xml-like syntax for creating elements (used in react mostly). So rather than launching separate language servers for a ts and tsx file we just use one.

See also kakoune-lsp/kakoune-lsp#211

Copy link
Owner

Choose a reason for hiding this comment

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

My problem with this change is that. Someone configuring their setup may not know about the enforced mapping and tries to configure a language server for the correct syntax/languageID javascript, which will not be picked up due to our mapping.

Maybe we can come up with a more generic solution for this.

Our solution should support setups that want to map all JS/TS files to JSX/TSX, ones that allow to separate both and ones that only want to configure servers for JS/TSX.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, not sure of a clean solution as vis's filetype plugin only exposes *.tsx files as typescript files

And as I said it's a superset and I don't think there's any other js/ts language server in active development that doesn't support jsx/tsx

Copy link
Owner

@fischerling fischerling Apr 12, 2024

Choose a reason for hiding this comment

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

Thanks for your contribution and the insights!

Applied as a2dc6b9.

My initial concern is invalid since we use the mapping only to determine the languageId passed to the server not to select a language server configuration.

We can still think about a more generic/complex solution if someone encounters an issue with this mapping.

Copy link
Owner

@fischerling fischerling left a comment

Choose a reason for hiding this comment

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

What is the reason for the _show_message change?

I think it has worse UX than using vis:message.
The new split window has to be forcefully closed using :q! because it has unsaved changes.

@git-bruh
Copy link
Contributor Author

What is the reason for the _show_message change?

I think it has worse UX than using vis:message. The new split window has to be forcefully closed using :q! because it has unsaved changes.

Yeah it's just a personal thing. I felt vis:message output was harder to follow as there was no highlighting and the previous output was clubbed together. :q! is indeed annoying, i use the ZQ binding instead

@fischerling
Copy link
Owner

@git-bruh let me know what you think about https://gitlab.com/muhq/vis-lspc/-/merge_requests/49.

I tried to make the way messages are presented configurable.
You should achieve your desired behavior by setting lspc.show_message = 'open' in your configuration.

@fischerling
Copy link
Owner

Thanks for your contribution.
Sorry that I made rebasing for you harder by amending you commits.

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.

2 participants