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

Change smithy language server to the one from AWS #6572

Merged
merged 1 commit into from
Apr 14, 2023
Merged

Change smithy language server to the one from AWS #6572

merged 1 commit into from
Apr 14, 2023

Conversation

zetashift
Copy link
Contributor

Changes the current smithy language server to use the one from: https://github.com/awslabs/smithy-language-server
I've used both just fine(AWS one with VSCode and Disneys with helix) but I believe the AWS one is a bit nicer for general usage.

I'll update the wiki LS install instructions if this PR gets approved.

@zetashift
Copy link
Contributor Author

I am hitting a problem where the "go-to definition" selects more than it should:

  • Press go to definition on a shape ( in my case GetCustomerInput )
  • It'll select a few more lines than it should
    image

(notice how @output is also selected, even tho it's not part of that shape).

Not entirely sure if this is a LS problem, I can also reproduce this on Disney's LSP

Copy link

@mtdowling mtdowling left a comment

Choose a reason for hiding this comment

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

I think any generally applicable changes from the Disney fork are upstreamed, so this LGTM. Since the goto definition issue is in both LSPs, I still think we should switch, but opening an issue on https://github.com/awslabs/smithy-language-server would be great!

Updating the installation instructions would be great too, but I know you're waiting on agreement for the PR.

@zetashift
Copy link
Contributor Author

I think any generally applicable changes from the Disney fork are upstreamed, so this LGTM. Since the goto definition issue is in both LSPs, I still think we should switch, but opening an issue on https://github.com/awslabs/smithy-language-server would be great!

Updating the installation instructions would be great too, but I know you're waiting on agreement for the PR.

Ah yes, sounds good. I'll open up an issue is that repo, thank you for your help :D.

Copy link
Member

@pascalkuthe pascalkuthe left a comment

Choose a reason for hiding this comment

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

LGTM

@pascalkuthe pascalkuthe added E-easy Call for participation: Experience needed to fix: Easy / not much S-waiting-on-review Status: Awaiting review from a maintainer. A-language-support Area: Support for programming/text languages labels Apr 14, 2023
@the-mikedavis the-mikedavis merged commit cd7ef57 into helix-editor:master Apr 14, 2023
@zetashift zetashift deleted the smithy-aws-ls branch April 14, 2023 21:48
@zetashift
Copy link
Contributor Author

zetashift commented Apr 14, 2023

Thank you for reviewing and merging! :D

I updated the wiki to clarify that we're using the one from AWS: https://github.com/helix-editor/helix/wiki/How-to-install-the-default-language-servers#smithy
Installation instructions remain unchanged because coursier is still being used.

Triton171 pushed a commit to Triton171/helix that referenced this pull request Jun 18, 2023
wes-adams pushed a commit to wes-adams/helix that referenced this pull request Jul 4, 2023
smortime pushed a commit to smortime/helix that referenced this pull request Jul 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-language-support Area: Support for programming/text languages E-easy Call for participation: Experience needed to fix: Easy / not much S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants