Skip to content

Avoid changing the sigils of files not bumped#90

Merged
Morriar merged 1 commit intomasterfrom
at-fix-bump-bug
Sep 28, 2021
Merged

Avoid changing the sigils of files not bumped#90
Morriar merged 1 commit intomasterfrom
at-fix-bump-bug

Conversation

@Morriar
Copy link
Contributor

@Morriar Morriar commented Jun 3, 2021

Let's consider this RBI file:

# file1.rbi
# typed: true
class A
  def foo(a, b, c); end
end

We can create a conflict of signature when turning this file to true:

# file2.rb
# typed: false
class A
  def foo(a, b); end
end

When this happens an error will be reported for both file1.rbi and file2.rb which prompted spoom bump to revert the file to typed: false even if it was not bumped in the first place.

This PR fixes this behaviour by reverting only the sigil for files that were bumped.

Fixes #85.

@Morriar Morriar requested a review from a team June 3, 2021 22:16
+ file1.rb

Run `spoom bump --from false --to true` to bump them
OUT
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to not even make this suggestion in the first place?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I honestly don't know how to do so.

One way would be to parse the error message for file2.rbi and see that the first help section points to file1.rb. But the help sections points to a bad method more often than the other way around.

Any idea?

Copy link
Contributor

@KaanOzkan KaanOzkan Jun 4, 2021

Choose a reason for hiding this comment

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

Isn't it safe to assume if an already true file gave a redefined without matching argument count error it would have to be the file inside help section? We can also make sure that the file inside help section was bumped.

This solution does assume typecheck passes before running spoom.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As per our conversation on Slack: let's merge as this and improve later 👍

Fixes #85.

Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
@Morriar Morriar merged commit 9c6454f into master Sep 28, 2021
@Morriar Morriar deleted the at-fix-bump-bug branch September 28, 2021 18:03
@shopify-shipit shopify-shipit bot temporarily deployed to production September 28, 2021 18:16 Inactive
@shopify-shipit shopify-shipit bot temporarily deployed to production December 8, 2021 21:43 Inactive
@Morriar Morriar added the bugfix Fix a bug label Mar 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugfix Fix a bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

spoom bump --dry modifies files

3 participants