Skip gem RBI check when sorbet/rbi/gems does not exist #2330
Conversation
lib/ruby_lsp/tapioca/addon.rb
Outdated
| gem_rbi_directory = File.join(state.workspace_path, "sorbet", "rbi", "gems") | ||
|
|
||
| return unless File.exist?(gem_rbi_directory) | ||
|
|
There was a problem hiding this comment.
A few things here:
- We already have these well known paths as constants on the
Tapiocaobject. Please see other usages of it. - This is the default value of the output folder for gem RBI generation, but it isn't necessarily the value that is being used by the end user. We should instead be checking the actual value in the config, etc.
There was a problem hiding this comment.
Thank you for highlighting this!
I noticed we were not using the ::Tapioca::DEFAULT_GEM_DIR constant in addon.rb and run_gem_rbi_check.rb either, so I've gone ahead and updated those.
We're now also checking the user's config file for any configured gem directory.
Please take a look and let me know if you have more feedback.
- Check if gem RBI directory exists before running gem RBI check - Respect user's configured gem directory from tapioca's config file Fixes the issue where the LSP addon would crash for new users who haven't yet generated any gem RBIs, as the gem RBI directory might not exist yet.
alexcrocha
force-pushed
the
ar/skip-run-rbi-check
branch
from
6c11bef to
377adf6
Compare
| end | ||
|
|
||
| #: (String) -> String | ||
| def gem_rbi_directory(workspace_path) |
There was a problem hiding this comment.
I am not sure I understand why this method is needed? The CLI command should be transparently get the correct value of the gem rbi directory as part of the CLI command options. That needs to be passed down here, not read the config from scratch.
There was a problem hiding this comment.
This is triggered by the Ruby LSP through the activate method. At this point we haven't shelled out to the Tapioca gem command yet, and the config hasn't been parsed by CLI.
Before we shell out, we need to do some cleanup. When we detect deleted or lingering RBIs (through git commands) we either restore them or delete the new ones, source.
We can either duplicate the config processing here in the add-on, or move this cleanup to the Tapioca gem command to be triggered optionally in add-on mode.
There was a problem hiding this comment.
The gem RBI command does that kind of clean up automatically anyway, so I am not even sure what these git operations are trying to do.
I think doing this outside of the gem RBI command is asking for trouble. I don't think we should be doing any config parsing, or relying on constant values to assume anything about where RBI files live, or which ones should be cleaned up, etc.
All that logic already exists. Please tell me what's missing that all this is needed.
There was a problem hiding this comment.
The tapioca gem command only runs when we detect lockfile differences. However, consider this scenario:
- Developer adds/modifies/removes a gem and regenerates RBIs
- Developer reverts the Gemfile changes and runs
bundle install - The lockfile returns to its committed state
- LSP restarts due to lockfile change, but finds no diff against the committed version
tapioca gem <gems>doesn't run, leaving orphaned RBI files or missing previously deleted ones
This is where the git operations come in: they clean up orphaned RBIs and restore deleted ones. While I understand the concern about duplicating logic, the add-on and CLI run in separate processes, so we must read the configuration independently.
We did consider the alternative of running tapioca gem on every activation to simplify this. However, we decided against it due to the potential negative impact on developer experience. For instance, if a developer pulls main and the latest commit didn't include updated gem RBIs, the add-on would immediately generate gem RBIs for files they didn't touch, polluting their workspace. We felt it was better to avoid this.
There was a problem hiding this comment.
But the gem command can clean up stale RBI files and create RBI files that should exist as well. I still don't understand why this level of detection needs to happen outside of the normal Tapioca gem RBI generation. We should be able to find a way to make the gem RBI generation to be smarter about this.
I don't think we want to try to duplicate the logic of configuration files, etc, outside of that flow. That won't scale.
There was a problem hiding this comment.
The tapioca gem command only runs when we detect lockfile differences. However, consider this scenario:
- Developer adds/modifies/removes a gem and regenerates RBIs
- Developer reverts the Gemfile changes and runs
bundle install- The lockfile returns to its committed state
- LSP restarts due to lockfile change, but finds no diff against the committed version
tapioca gem <gems>doesn't run, leaving orphaned RBI files or missing previously deleted ones
This scenario is exactly why the git based detection is not a great idea in the first place, and not a good reason to add more complexity to this. There is no good way to synchronize the state of RBI files with the state of the lockfile changes.
2 participants
Closes #2308
Motivation
When setting up a new repo with Tapioca, the configured gem RBI directory might not exist yet, and loading the Tapioca add-on crashes.
Implementation
The implementation now checks for the configured gem RBI directory. It then verifies this directory exists before running the gem RBI check, preventing the original crash.
Previously, the implementation only used the default gem RBI directory. I have updated our implementation to correctly respect the user's configuration.