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

Helpers #21

Merged
merged 4 commits into from
Dec 23, 2016
Merged

Helpers #21

merged 4 commits into from
Dec 23, 2016

Conversation

sethjeffery
Copy link
Collaborator

From the README...

Helper methods

You can define helpers with the helper keyword. Helpers can be used within blocks, procs, or as symbolic names for if statements to tidy up your code.

Flagship.define :blog do
  helper :is_author do |comment, user|
    comment.author == user
  end

  def can_view_comment(context)
    context.current_user.moderator?
  end

  enable :comment, if: :can_view_comment
  enable :comment_deletion, if: ->(context) { is_author(context.comment, context.current_user) }
end

To share helpers, you can simply include them as modules.

module FlagHelpers
  def is_author(context)
    context.comment.author == context.current_user
  end
end

Flagship.define :development do
  include FlagHelpers
  enable :delete, if: :is_author
end

Flagship.define :production do
  include FlagHelpers
  enable :delete, if: :is_author
end

Copy link
Owner

@yuya-takeyama yuya-takeyama left a comment

Choose a reason for hiding this comment

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

@sethjeffery
Awesome, I wanted this feature!

But just to confirm, are there any differences between a method defined by helper and one defined by basic def?

@sethjeffery
Copy link
Collaborator Author

sethjeffery commented Dec 23, 2016

Yes the main difference is that helper methods are kept within the instance context of the flagset, but if you define by def it is stored in the Flagship global context shared with all flagsets, which I find a bit dangerous because you can accidentally overwrite definitions.

Modules included with include have been tested to be safe, because it actually uses extend and stores at the instance level.

@yuya-takeyama
Copy link
Owner

@sethjeffery

if you define by def it is stored in the Flagship global context shared with all flagsets

Oh, that's dangerous...
But I couldn't see such a situation.

require 'flagship'

Flagship.define :a do
  def foo(c)
    puts "foo is called"
    true
  end
end

Flagship.define :b do
  enable :feature, if: ->(c) { foo(c) }
end

Flagship.select_flagset(:b)

p Flagship.enabled?(:feature)
$ bundle exec ruby test.rb
test.rb:11:in `block (2 levels) in <main>': undefined method `foo' for #<Flagship::Dsl:0x007f96e3223090> (NoMethodError)
Did you mean?  fork
        from /Users/yuya/src/github.com/sethjeffery/flagship/lib/flagship/feature.rb:24:in `enabled?'
        from /Users/yuya/src/github.com/sethjeffery/flagship/lib/flagship/flagset.rb:17:in `enabled?'
        from /Users/yuya/src/github.com/sethjeffery/flagship/lib/flagship.rb:20:in `enabled?'
        from test.rb:16:in `<main>'

@sethjeffery
Copy link
Collaborator Author

Hmm you're right! I actually had originally written to use def but I encountered problems so I changed it... now I'm not sure how to reproduce the problems! (Maybe I fixed it through another change?)

@yuya-takeyama
Copy link
Owner

Maybe by this change?
2817e99

Then I think we don't need helper keyword.
Basically I want to keep the library small.

@sethjeffery
Copy link
Collaborator Author

Ok I'll remove the keyword.

Copy link
Owner

@yuya-takeyama yuya-takeyama left a comment

Choose a reason for hiding this comment

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

LGTM!
Thank you!

@yuya-takeyama yuya-takeyama merged commit a4afc13 into yuya-takeyama:master Dec 23, 2016
@yuya-takeyama
Copy link
Owner

I'll release this with one more change later today.

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