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

Add CONTRIBUTING.md #3209

Merged
merged 1 commit into from
May 19, 2021
Merged

Add CONTRIBUTING.md #3209

merged 1 commit into from
May 19, 2021

Conversation

tsmartt
Copy link
Contributor

@tsmartt tsmartt commented Apr 30, 2021

This is meant to be a set of guidelines for anyone contributing to follow, and this PR is meant to spark constructive criticism.
Please feel free to propose additions where any might be lacking, and modifications with good justification.

This is meant to be a set of guidelines for anyone contributing to
follow, and this PR is meant to spark constructive ctriticism.
Please feel free to propose additions where any might be
lacking, and modifications with good justification.
- follow Sandi Metz' [rules][sm]
- prefer [composition over inheritance][composition]

[composition]: https://betterprogramming.pub/prefer-composition-over-inheritance-1602d5149ea1
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO the article is dogmatic, other's like such as https://thoughtbot.com/blog/reusable-oo-composition-vs-inheritance suggest that the developer should understand the trade-off between composition vs inheritance.

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 agree, that's why I put prefer.

@tsmartt
Copy link
Contributor Author

tsmartt commented May 3, 2021

Another thought I had was mimicking the behavior of mutations and interactor, but which a much simpler setup, something like:

# frozen_string_literal: true
module My
    class Service < BaseService
        class COMMAND
            include ActiveModel::Validations
            validates_presence_of :param1
            validates_presence_of :param2
        end
      def self.build
        new(tp_need: TPNeed)
      end
      def initialize(tp_need:)
        @tp_need = tp_need
      end
      def execute(param1:, param2)
        result = @tp_need.do_something(param1, param2)
        if not result
            return fail("Some error message")
        end
        success("Did thing")
      end
    end
  end
end

The base class would be something like 40 lines of ruby and would give us what we need with no extra frameworks. It instantiates the COMMAND class with the parameters and validates, if validation passes, it calls execute.
By passing the params both to self.build and execute, we are free to use ruby's type system features to help stabilize the code. To invoke it you just My::Service.build.call(param1: '1', param2: '2')

For most things you could probably even leave out the COMMAND -- since the type system could take care of basic things like non-nil, etc.. could add it in for more complex scenarios like email validation.

But we can leave this for a followup-PR.

@tsmartt
Copy link
Contributor Author

tsmartt commented May 18, 2021

@yachtcaptain23 going to merge this tomorrow unless any further thoughts.

@tsmartt tsmartt merged commit de0f80e into brave-intl:staging May 19, 2021
@tsmartt tsmartt deleted the add-contributing branch May 19, 2021 18:00
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.

2 participants