Skip to content

Conversation

@alexcrocha
Copy link
Contributor

@alexcrocha alexcrocha commented Dec 5, 2024

Motivation

Currently, when a gem specified in the tapioca gem command isn't found by bundler, the process fails with a Thor::Error. This creates a poor user experience on the rare instances when the Tapioca LSP Addon runs the command with multiple gems where some might not be available.

Implementation

With this change, when the --lsp_addon flag is used, the command will now output a warning when a gem isn't found and continue processing other gems instead of crashing the process. This allows the Tapioca LSP Addon to handle missing gems gracefully while still generating RBIs for the available ones.

Without --lsp_addon, the command maintains its existing behaviour of failing with an error when a gem cannot be found.

@alexcrocha alexcrocha added the enhancement New feature or request label Dec 5, 2024
@alexcrocha alexcrocha requested a review from a team as a code owner December 5, 2024 01:42
@rzane
Copy link
Contributor

rzane commented Dec 5, 2024

I'm not a maintainer, but I want to call out that this change would allow (gem_names - @excluded) to change back to gem_names.

See: #2096

@andyw8
Copy link
Contributor

andyw8 commented Dec 5, 2024

This creates a poor user experience, especially when running the command with multiple gems where some might not be available.

Can you explain some more about this use case? If I entered an invalid gem name, I would expect tapioca to stop (and exit with a useful error).

@andyw8
Copy link
Contributor

andyw8 commented Dec 5, 2024

(and let's add a test if we choose to proceed)

@KaanOzkan
Copy link
Contributor

Can you explain some more about this use case? If I entered an invalid gem name, I would expect tapioca to stop (and exit with a useful error).

LSP add-on will supply gem names very leniently and as a result can hit this exception often. We think it's cleaner to fix it here than rescue exceptions.

In your use case we'll still exit without doing much work

(and let's add a test if we choose to proceed)

Yes we need a CLI tests to accompany this change.

@andyw8
Copy link
Contributor

andyw8 commented Dec 5, 2024

Got it. Can we detect if running through the LSP or not, and respond accordingly?

@alexcrocha alexcrocha force-pushed the ar/handle-no-gem-gracefully branch 2 times, most recently from b8e73c7 to 2d8c0ee Compare December 7, 2024 01:50
@alexcrocha alexcrocha changed the title Handle missing gems gracefully in tapioca gem command [Tapioca Addon] Handle missing gems gracefully in tapioca gem command Dec 7, 2024
gem = @bundle.gem(gem_name)

if gem.nil?
next say("Warning: Cannot find gem '#{gem_name}', skipping", :yellow) if @lsp_addon
Copy link
Contributor

Choose a reason for hiding this comment

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

(nit) Since say doesn't return anything meaningful, I think we shouldn't pass is it to next.

Suggested change
next say("Warning: Cannot find gem '#{gem_name}', skipping", :yellow) if @lsp_addon
if @lsp_addon
say("Warning: Cannot find gem '#{gem_name}', skipping", :yellow)
next
end

Copy link
Contributor

Choose a reason for hiding this comment

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

Another point: I think someone else brought this up before but if we're going to make this specific to lsp_addon I'd be okay with not outputting the warning log.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's fair. It does create a bit of noise that might not be very relevant.

I'll remove the warning log.

This flag allows us to detect when the gem command is executed by the
Tapioca LSP Addon.
@alexcrocha alexcrocha force-pushed the ar/handle-no-gem-gracefully branch from 2d8c0ee to 57f0d07 Compare December 18, 2024 18:46
@alexcrocha alexcrocha force-pushed the ar/handle-no-gem-gracefully branch from 57f0d07 to 48799d0 Compare December 18, 2024 19:11
@alexcrocha alexcrocha merged commit 14c955d into main Dec 18, 2024
28 checks passed
@alexcrocha alexcrocha deleted the ar/handle-no-gem-gracefully branch December 18, 2024 20:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants