Warning when running Sorbet

sandstrom commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

@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?

twitter / twitter-cldr-rb

Warning when running Sorbet #252