Include excluded gems in verify logic #1058
Conversation
lib/tapioca/commands/gem.rb
Outdated
| diff = {} | ||
|
|
||
| removed_rbis.each do |gem_name| | ||
| filename = existing_rbi(gem_name) | ||
| diff[filename] = :removed | ||
| unless exclude.include?(gem_name) |
There was a problem hiding this comment.
Do we need to do the same in the loop below for added_rbis?
There was a problem hiding this comment.
exclude prevents the creation of an RBI for the excluded gem and if one exists, deletes it so my understanding is that there really shouldn't be a case where an excluded RBI ends up in theadded_rbis array
| @@ -1604,30 +1604,23 @@ class User; end | |||
| assert_success_status(result) | |||
| end | |||
|
|
|||
| it "advises of removed file(s) and returns exit_status 1" do | |||
| it "is aware of exclude option and does not error due to removed files" do | |||
There was a problem hiding this comment.
I think the previous test has value. Can we add a new example instead of modifying the existing one?
There was a problem hiding this comment.
Yeah I can do that! I am a bit confused though because this code change means there won't be a warning of removed file(s) & won't return exist_status 1 with gem --verify --exclude foo bar so I'm not sure how to leave this test unmodified since what it is testing is no longer the expected behaviour.
There was a problem hiding this comment.
The test was probably using exclude to simplify the setup, but the scenario is still valid. We should still warn users of removed gems and exit with status 1.
Instead of running Tapioca with the exclude flag, you'll need to somehow remove the fake foo or bar gems from @project and then see that the RBI is removed.
There was a problem hiding this comment.
Thanks for the clarification!
There was a problem hiding this comment.
I believe we already have a test for that scenario here:
tapioca/spec/tapioca/cli/gem_spec.rb
Lines 1609 to 1642 in 67f9504
There was a problem hiding this comment.
Oh, okay. In that case, I think we're covered. Thanks for looking into it!
lib/tapioca/commands/gem.rb
Outdated
| diff = {} | ||
|
|
||
| removed_rbis.each do |gem_name| | ||
| filename = existing_rbi(gem_name) | ||
| diff[filename] = :removed | ||
| unless exclude.include?(gem_name) |
There was a problem hiding this comment.
Can we instead do:
| unless exclude.include?(gem_name) | |
| next if exclude.include?(gem_name) |
so that the code is easier to read and not deeply nested?
adisonlampert
force-pushed
the
al/include-exclude-in-verify-logic
branch
2 times, most recently
from
a7692db
to
3a148e9
Compare
|
@stanjdev no LOL that was a mistake 🤣 I'm googling how to remove it |
adisonlampert
force-pushed
the
al/include-exclude-in-verify-logic
branch
from
3a148e9
to
aa2c443
Compare
Motivation
Closes #658
Excluded gems shouldn't be verified.
Implementation
I went into the
verifyimplementation and added in some logic to skip over anyexcludedgems so that it doesn't error due to them being removed.Tests
I updated the related test and all local tests passed.
Additionally, I spun up an instance of
Shopify/shopifyand added a random gem (colorize) to the list of excluded gems thenbin/tapioca gemto remove thecolorizegem andbin/tapioca --verifywhich caused no errors. Once I removedcolorizefrom the excluded list,bin/tapioca --verifyerrored as expected.