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

Diagnostic and codeActions for missing/outdated Smithy version #86

Merged
merged 15 commits into from
Jan 17, 2023

Conversation

daddykotex
Copy link
Contributor

@daddykotex daddykotex commented Jan 12, 2023

We were wondering what would be a good way to push users towards adding the $version statement in their Smithy file. By default, if it's missing, smithy-model assumes 1. But most Smithy files could be using 2 so we discussed a way to have this in the IDE.

The flow is basically: if you don't have a version, we suggest/help to define it explicitly (to 1), and if you use version 1, we suggest/help to update it to 2.

The end result looks like this in VS Code:

lsp-smithy-version.mov

It works by:

  1. publishing a diagnostic if the version is missing
  2. publishing a diagnostic if the version is found but it contains an older version (1 here)
  3. implement codeAction to generate TextEdit if the diagnostic above are found

For 1, and 2, we read the top part of the Smithy file (in this PR I read 5 lines but this is an arbitrary amount). Then we look for a line starting with $version. If not found we publish a diagnostic, if found but equal to 1, we publish a diagnostic.

Those diagnostic are added on top of any diagnostic generated from validating the model.

The TextEdit are simple:

  • rewrite the version if it is found (we can reuse the range from the diagnostic to build the TextEdit)
  • append $version: "1" at the top of the file, explicitly setting the version for the user

On top of that, this PR includes (those could be separated PRs):

  • a typo fix in some java doc
  • a fix when using findToken on an empty document

@daddykotex daddykotex requested a review from a team as a code owner January 12, 2023 15:01
@srchase
Copy link
Contributor

srchase commented Jan 12, 2023

This is great.

What's the rational for defaulting to 1 instead of 2 when no version is defined?

It looks like the 1 to 2 upgrade is suggested regardless of whether the model will work as a 2 model. Is the intent that they will get diagnostics on anything that isn't compatible with 2 and be able to make the other needed changes on their own?

It'd be awesome if the CodeAction could handle making more of those changes with a preview of what it would change, but maybe that could be added later. I think at a minimum, it'd be helpful to have the 1 to 2 suggestion include some additional context. Perhaps link to the migration guide?

@daddykotex
Copy link
Contributor Author

What's the rational for defaulting to 1 instead of 2 when no version is defined?

My understanding is that if no version is defined, 1 is assumed. With that in mind, we figured that the lowest friction way to be to let the user know that they're out of date. We assume that if users did not define a version it's because they don't know they should have one. So we suggest to add one, if they do, they get the version their file is at. That means no extra warnings about deprecated stuff and what not. But they see suggestion to upgrade so they can do that.

We think making it a two step will reduce the confusion related to changes between versions (changes that could show up in the editor like warnings or errors).

@daddykotex
Copy link
Contributor Author

It'd be awesome if the CodeAction could handle making more of those changes with a preview of what it would change, but maybe that could be added later. I think at a minimum, it'd be helpful to have the 1 to 2 suggestion include some additional context. Perhaps link to the migration guide?

Do you mean that it could be valuable to have a custom action to update all files (to insert a version, or update a version)?

I think at a minimum, it'd be helpful to have the 1 to 2 suggestion include some additional context.

Good idea, I'll see if I can add that

@srchase
Copy link
Contributor

srchase commented Jan 12, 2023

Do you mean that it could be valuable to have a custom action to update all files (to insert a version, or update a version)?

I meant that for a given file, the code action which is changing 1 to 2 would also be able to do the 1 to 2 migration on that file. For example, it could detect set shapes and change them to list shapes.

@daddykotex
Copy link
Contributor Author

Do you mean that it could be valuable to have a custom action to update all files (to insert a version, or update a version)?

I meant that for a given file, the code action which is changing 1 to 2 would also be able to do the 1 to 2 migration on that file. For example, it could detect set shapes and change them to list shapes.

Ahhh ok I see what you mean. I think I prefer to scope code action per diagnostic. In practice that means that once you move to version 2. You'll get a new set of diagnostic. If you use set you'd get one for that. We could have an independent code action that does that: turn set into uniqueItems on list. Similarly, we could have a codeaction to rewrite enum I believe.

I think at a minimum, it'd be helpful to have the 1 to 2 suggestion include some additional context.

Regarding the note, I think I can add some context to the diagnostic, but not the code action. See the following examples from the Shellcheck extension:

Screen Shot 2023-01-12 at 13 47 34

Screen Shot 2023-01-12 at 13 47 27

@srchase
Copy link
Contributor

srchase commented Jan 12, 2023

Regarding the note, I think I can add some context to the diagnostic, but not the code action. See the following examples from the Shellcheck extension:

Adding to the diagnostic sounds good.

@daddykotex
Copy link
Contributor Author

Implemented the href code description:

Screen Shot 2023-01-12 at 15 20 12

It requires an update on the client side because it's only supported as of LSP 3.16.0

@milesziemer
Copy link
Contributor

This is awesome, thanks for doing it. Would you mind splitting up the other changes you mentioned into separate PR's so it is easier to review?

* updateSmithyVersion => shows up when a diagnostic for outdated
Smithy version is present and it update the version to the latest
* defineSmithyVersion => shows up when a diagnostic for missing
Smithy version is present and it adds the version statement at
the top of the file
Make the function work on a single file, and use that function
from within the `createPerFileDiagnostics` rather than on top of
it. This allows us to avoid two PublishDiagnosticsParams for
one file.
This requires the version 3.16 of the LSP and thus required me
to update the `vscode-languageclient` to 7.0.0.
@daddykotex daddykotex force-pushed the dfrancoeur/smithy-version branch from 14ca4e8 to 29f3bfd Compare January 13, 2023 17:27
@daddykotex
Copy link
Contributor Author

Refactored code action and diagnostic for the version in their own file and created a test suite for the whole version refactoring feature (code action and diagnostic for version)

@srchase srchase merged commit 3245020 into smithy-lang:main Jan 17, 2023
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.

3 participants