Skip to content
This repository has been archived by the owner on May 30, 2024. It is now read-only.

Remove experimental variable guessing in Ruby grammar #962

Merged
merged 1 commit into from
Jan 8, 2024

Conversation

ryanb
Copy link
Contributor

@ryanb ryanb commented Jan 5, 2024

Motivation

The grammar file incorrectly assumes most top level method calls are variables.

230160734-f81fc579-5ff1-4760-b0db-3dc5dda01ee7

There is a comment in the grammar labeling this as "experimental".

This is kindof experimental. There really is no way to perfectly match all regular variables, but you can pretty well assume that any normal word in certain curcumstances that haven't already been scoped as something else are probably variables, and the advantages beat the potential errors

I don't agree with "any normal word in certain curcumstances that haven't already been scoped as something else are probably variables" because method calls fall under this. I'd rather we error on the side of not scoping something we don't know.

When the LSP is active it adds semantic highlighting tokens which properly discerns local variables from method calls. It is better to use this instead of the grammar to determine local variables.

See further discussion in Shopify/ruby-lsp#1573.

Implementation

I removed this experimental part of the grammar so it no longer scopes method calls as variable.other.ruby.

Automated Tests

There are no automated tests for the grammar file.

Manual Tests

You can check this by opening a Ruby file, putting text cursor in a method call, and running the "Inspect Editor Tokens and Scopes" command. It should not show variable.other.ruby under textmate scopes section.

@ryanb ryanb requested a review from a team as a code owner January 5, 2024 00:44
@ryanb ryanb requested review from egiurleo and KaanOzkan January 5, 2024 00:44
@KaanOzkan KaanOzkan requested review from vinistock and removed request for KaanOzkan January 5, 2024 14:49
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.

Agreed. We haven't revisited this grammar, but I the ideal state would be to not push any scopes for ambiguous syntax

@vinistock vinistock merged commit 9e87d70 into Shopify:main Jan 8, 2024
6 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants