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

Add Prism warnings in diagnostics #1437

Merged
merged 2 commits into from
Mar 1, 2024
Merged

Conversation

snutij
Copy link
Contributor

@snutij snutij commented Feb 29, 2024

Motivation

Closes #1406

Add Prism warnings in diagnostics, actually we handle only errors.

Implementation

Same as error, but based on parse_result.warnings

Some minor changes in DiagnosticsExpectationsTest because we are not handling only Rubocop diagnostics now.

Automated Tests

Added fixtures and expectations.

Manual Tests

You can open test/fixtures/syntax_diagnostics.rb and see errors and warning.

image

image

@snutij snutij requested a review from a team as a code owner February 29, 2024 20:43
@snutij snutij requested review from Morriar and vinistock February 29, 2024 20:43
@snutij snutij force-pushed the show-prism-warning branch from 357f426 to f77c918 Compare February 29, 2024 21:25
@andyw8 andyw8 added the enhancement New feature or request label Feb 29, 2024
@andyw8
Copy link
Contributor

andyw8 commented Feb 29, 2024

FYI @kddnewton

@andyw8
Copy link
Contributor

andyw8 commented Feb 29, 2024

I wonder if we should make this configurable (#1434) since if working on a legacy there project, there could be lot of noise.

Also, are we good with cases such as this where the Prism and RuboCop warnings covers the same thing, e.g.: (I see it was discussed in the issue).

Screenshot 2024-02-29 at 4 43 21 PM

Copy link
Member

@st0012 st0012 left a comment

Choose a reason for hiding this comment

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

I agree that this could be a configurable feature, which can be the first new config of .ruby-lsp.yml.

That said, I think it's fine to merge or even release this first to see how people react to it.

@kddnewton
Copy link
Contributor

I don't think there will be much noise to be honest, there aren't that many kinds of warnings and all of them are trivially fixable.

I do think you could add a configuration option to limit it to "default"-level warnings versus "verbose"-level warnings though.

Copy link
Member

@vinistock vinistock left a comment

Choose a reason for hiding this comment

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

Just a minor nitpick, but this is great 🚀.

Concerning configuring this, I honestly wouldn't worry about it ahead of time. If people start complaining about the warnings we can consider it, but I don't think it warrants increasing complexity.

However, I wonder if we can now shutdown some of the RuboCop rules in the rubocop-shopify configuration. Prism is surely going to be a lot faster in surfacing this and we can reduce the number of cops running.

@kddnewton do you have a list of which warnings are available? (Even better if you know which RuboCop rules they map to).

lib/ruby_lsp/requests/diagnostics.rb Outdated Show resolved Hide resolved
@snutij snutij force-pushed the show-prism-warning branch from f77c918 to a9dc4bc Compare March 1, 2024 15:38
@kddnewton
Copy link
Contributor

@vinistock they're listed here: https://github.com/ruby/prism/blob/f41ba6d7ecfc844535996e24525d4ea47c0a85db/src/diagnostic.c#L305-L314.

I haven't taken the time to line them up 1-to-1 with warnings from rubocop, but if you don't mind doing that it would actually help me. I need it for ruby/prism#2513.

@andyw8
Copy link
Contributor

andyw8 commented Mar 1, 2024

I'll take a stab at matching those to the RuboCop cops.

@andyw8
Copy link
Contributor

andyw8 commented Mar 1, 2024

[PM_WARN_AMBIGUOUS_FIRST_ARGUMENT_MINUS] -> Lint/AmbiguousOperator
[PM_WARN_AMBIGUOUS_FIRST_ARGUMENT_PLUS]  -> Lint/AmbiguousOperator
[PM_WARN_AMBIGUOUS_PREFIX_STAR]          -> Lint/AmbiguousOperator
[PM_WARN_AMBIGUOUS_SLASH]                -> Lint/AmbiguousRegexpLiteral
[PM_WARN_DUPLICATED_HASH_KEY]            -> Lint/DuplicateHashKey
[PM_WARN_DUPLICATED_WHEN_CLAUSE]         -> Lint/DuplicateCaseCondition
[PM_WARN_EQUAL_IN_CONDITIONAL]           -> Lint/AssignmentInCondition
[PM_WARN_END_IN_METHOD]                  -> Lint/Syntax
[PM_WARN_FLOAT_OUT_OF_RANGE]             -> Lint/FloatOutOfRange
[PM_WARN_INTEGER_IN_FLIP_FLOP]           -> Lint/FlipFlop

@vinistock
Copy link
Member

Thanks for the contribution!

@vinistock vinistock merged commit 00c42a2 into Shopify:main Mar 1, 2024
21 checks passed
@vinistock vinistock added the server This pull request should be included in the server gem's release notes label Mar 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request server This pull request should be included in the server gem's release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Show Prism warnings in diagnostics
5 participants