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

Warning when running Sorbet #252

Open
sandstrom opened this issue Apr 11, 2022 · 2 comments
Open

Warning when running Sorbet #252

sandstrom opened this issue Apr 11, 2022 · 2 comments

Comments

@sandstrom
Copy link
Contributor

I've tried out running sorbet on our code base.

I've run into the following problem stemming from Twitter CLDR.

sorbet/rbi/hidden-definitions/hidden.rbi:175756: Method TwitterCldr::Segmentation::StateMachine.instance redefined without matching argument count. Expected: 0, got: 2 https://srb.help/4010
      175756 |  def self.instance(boundary_type, locale); end
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    sorbet/rbi/hidden-definitions/hidden.rbi:175730: Previous definition
      175730 |  include ::Singleton
                ^^^^^^^^^^^^^^^^^^^

Same error for TwitterCldr::Segmentation::StateMachine.instance, TwitterCldr::Segmentation::Suppressions.instance, TwitterCldr::Shared::DayPeriods.instance and TwitterCldr::Timezones::Timezone.instance. This search will also bring up the relevant files: https://github.com/twitter/twitter-cldr-rb/search?q=instance+singleton

I guess one solution could be to rename the instance method, so it doesn't collide with instance method defined via Singleton module:

      def locale_instance(locale)
          instance_cache[locale] ||= new(locale)
        end

Another solution would be to not use the Singleton module, but that's likely more work.

If you'd accept a PR I can try to make a change where instance is renamed to locale_instance (or maybe cached_instance).

@camertron
Copy link
Collaborator

Hey @sandstrom, this is an interesting one! Let's rename the methods to instance_for, which is more in-line with the conventions used in the library. Since we're renaming, it doesn't make sense to include Singleton anymore, since we'd have to mark the original instance method as private and I'm not sure if Sorbet would take kindly to that (in any case, it's weird). AFAIK the Singleton module only does a couple of other nice things like mark new as private. It shouldn't be difficult to replicate the behavior.

@sandstrom
Copy link
Contributor Author

sandstrom commented Apr 12, 2022

@camertron Sounds like a good plan!

Yeah, you're right, doesn't do that much.

Would this be a breaking change, or is it only an internal refactoring?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants