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

feat: remove super() checks in initializers #12

Closed
wants to merge 1 commit into from

Conversation

sha-hil
Copy link

@sha-hil sha-hil commented Apr 23, 2024

The super() in initializers in child classes are not always required. Hence, I believe this should be optional and should not be picked up by RC.

Example code where RC asks for super() but it is not required.

# frozen_string_literal: true

module Loyalty
  class V6LoyaltyRewardsCard
    class Base
      def state
        raise NotImplementedError, "Override in subclass"
      end

      def state_attributes
        raise NotImplementedError, "Override in subclass"
      end

      def activity_history
        {
          text: I18n.t("everyday_rewards.rewards_screen.activity_history_text"),
          url:  Settings.everyday_rewards.activity_history_url

        }
      end
    end

    class Empty < Base
      def state
        "LOYALTY_EMPTY"
      end

      def state_attributes
        {
          primary_text: I18n.t("everyday_rewards.rewards_screen.loyalty_empty_text")
        }
      end

      def activity_history
        raise NoMethodError, "Empty rewards card does not have access to activity_history"
      end
    end

    class Operating < Base
      def initialize(everyday_rewards_id:, balances:, cpl_discount:)
        @everyday_rewards_id = everyday_rewards_id
        @balances = balances
        @cpl_discount = cpl_discount
      end

      def state
        "LOYALTY_OPERATING"
      end

      def state_attributes
        {
          everyday_rewards_id: @everyday_rewards_id,
          cpl_discount: @cpl_discount,
          activity_history: activity_history,
          balances: @balances
        }
      end
    end

    class Error < Base
      def initialize(error_text:)
        @error_text = error_text
      end

      def state
        "LOYALTY_ERROR"
      end

      def state_attributes
        {
          primary_text: @error_text,
          error_image_url: "#{ApplicationConfig::EVERYDAY_REWARDS_IMAGE_BASE_URL}/#{Settings.everyday_rewards.rewards_error_image}",
          activity_history: activity_history
        }
      end
    end
  end
end

The super() is not always required when Rubocop picks up on it. I think making it optional is a good idea.
Happy to discuss.
@sha-hil sha-hil self-assigned this Apr 23, 2024
@sha-hil sha-hil requested review from invke and LuyiPK April 23, 2024 23:04
@sha-hil sha-hil changed the title feat: remove super requirement in initializers feat: remove super() checks in initializers Apr 23, 2024
Copy link

@LuyiPK LuyiPK left a comment

Choose a reason for hiding this comment

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

LGTM, thanks Sha 👍

Copy link
Contributor

@invke invke left a comment

Choose a reason for hiding this comment

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

Gonna challenge this, this is typically a syntax error on compiled languages, so I'm hesitant for us to remove a lint guard for it. See the thinking behind why this rule exists: rubocop/ruby-style-guide#809

rubocop/ruby-style-guide#809 (comment)

@silva96 In this particular example, it does nothing and perhaps the super call is not needed. The problem is that you don't know this is the case if the superclass is part of another library or maintained by someone else. The base class may add some initialization logic in the future that would not get run by your subclass.

It is worth pointing out that the Java language has required all constructors to call a superconstructor since the beginning for exactly this reason: not calling the superclass constructor breaks the guarantee that it will run when instances of that class or its subclasses are instantiated. Java code requires that the subclass invoke a superclass constructor directly, or else a call to the superclass's default (no-args) constructor will be inserted for you.

@sha-hil
Copy link
Author

sha-hil commented Apr 28, 2024

Ah yup I get it. It's better it remains for the situations like working with gems etc and a super() is needed. Cheers for that @invke !

@sha-hil sha-hil closed this Apr 28, 2024
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

Successfully merging this pull request may close these issues.

None yet

3 participants