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

Update Prism requirement to v0.28 #2017

Merged
merged 10 commits into from
May 16, 2024
Merged

Update Prism requirement to v0.28 #2017

merged 10 commits into from
May 16, 2024

Conversation

vinistock
Copy link
Member

Motivation

Update our Prism requirement to v0.28 or higher. Version v0.28 includes the new ASCIISource with performance optimization for sources that do not contain multibyte characters, which fixes the performance regression we introduced in semantic highlighting.

Implementation

  1. Updated Prism and RBI
  2. Re-generated gem RBIs
  3. Fixed errors related to the restructuring of constant paths

Before, each part of a constant path was also a constant read. After this version, only the parent parts of the constant path may be constant reads, the main name of the constant is embedded inside the constant path node.

# Before
# This would be 3 constant read nodes to compose the constant path

# After
# This is now 2 constant read nodes and the name `C` is a part of the overall constant path
A::B::C

Automated Tests

Fixed a few tests.

@vinistock vinistock added bugfix This PR will fix an existing bug server This pull request should be included in the server gem's release notes labels May 6, 2024
@vinistock vinistock self-assigned this May 6, 2024
@vinistock vinistock requested a review from a team as a code owner May 6, 2024 18:53
@vinistock vinistock requested review from andyw8 and st0012 May 6, 2024 18:53
@vinistock vinistock force-pushed the vs/update_prism branch 15 times, most recently from 2fa5a9b to 1618f36 Compare May 9, 2024 15:05
@vinistock vinistock force-pushed the vs/update_prism branch 2 times, most recently from 0316673 to 54add89 Compare May 9, 2024 18:18
@andyw8
Copy link
Contributor

andyw8 commented May 10, 2024

Are the vscode/ changes supposed to be part of this PR?

@vinistock
Copy link
Member Author

The upgrade ended up uncovering some other Windows errors and I was trying to get CI to pass. If you prefer, I can split that into a separate PR, but the changes are necessary.

@andyw8
Copy link
Contributor

andyw8 commented May 10, 2024

The upgrade ended up uncovering some other Windows errors and I was trying to get CI to pass. If you prefer, I can split that into a separate PR, but the changes are necessary.

Ok, yeah I'd say we should split them since there's quite a lot of changes. I'll limit my review to the Ruby part.

@vinistock
Copy link
Member Author

Split the other part, which needs to be merged first #2033.

@andyw8
Copy link
Contributor

andyw8 commented May 16, 2024

#2033 is blocked on Windows, so I'll update this branch to remove the /vscode changes.

@@ -86,6 +86,8 @@ jobs:
# We need some Ruby installed for the environment activation tests
- name: Set up Ruby
uses: ruby/setup-ruby@v1
with:
bundler-cache: true
Copy link
Contributor

Choose a reason for hiding this comment

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

👏

@andyw8 andyw8 enabled auto-merge (squash) May 16, 2024 18:53
@andyw8 andyw8 merged commit e3c0f20 into main May 16, 2024
34 checks passed
@andyw8 andyw8 deleted the vs/update_prism branch May 16, 2024 19:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This PR will fix an existing bug 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.

2 participants