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.

Sign up for GitHub

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

Jump to bottom

use Forwardable for all the feature flag delegation #1623

Merged
merged 1 commit into from
Oct 13, 2017
Merged

use Forwardable for all the feature flag delegation #1623

merged 1 commit into from
Oct 13, 2017

Conversation

robbkidd
Copy link
Contributor

@robbkidd robbkidd commented Jun 19, 2017 •
edited
Loading

Be clear about:

  1. Cannot instantiate a Feature object (module instead of class)
  2. List of methods delegated to the "feature flag adapter" which happens to be Rollout in our implementation.

Thanks to @cupakromer and @keithrbennett for helpful suggestions.

@robbkidd robbkidd requested review from thommay and iennae June 19, 2017 18:13
@jjasghar
Copy link

Walking through the code, it the less verbose makes more sustainable sense. The before merging though I suggest a comment or two explaining the module_function and def_delegators and the logic in using them. Also, adding a passing and expected fail test around this can with the logic explanation.

@robbkidd robbkidd force-pushed the robb/super-forwardable-feature branch from 68cfda6 to 77589c1 Compare August 16, 2017 22:20
@robbkidd robbkidd requested a review from a team August 16, 2017 22:20
@robbkidd robbkidd force-pushed the robb/super-forwardable-feature branch from 77589c1 to 931c726 Compare August 16, 2017 22:26
@robbkidd robbkidd force-pushed the robb/super-forwardable-feature branch from 931c726 to d15e3a2 Compare October 13, 2017 12:26
iennae
iennae suggested changes Oct 13, 2017
View reviewed changes
Copy link
Contributor

@iennae iennae left a comment

Choose a reason for hiding this comment

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

After pairing, requesting update to commit message to explain a bit more based on discussion as was really useful to understand. Also add a comment above module_function to note how to add an additional delegator.

@robbkidd robbkidd force-pushed the robb/super-forwardable-feature branch from d15e3a2 to e0120fb Compare October 13, 2017 19:01
@robbkidd
use Forwardable for all the feature flag delegation
7822895 
Be clear about:

1. Cannot instantiate a Feature object (module instead of class)
2. List of methods delegated to the "feature flag adapter" which
   happens to be Rollout in our implementation. This uses
   `def_delegators` to define several methods that are delegated
   to methods on Feature.adapter with the same name.
   `def_delegators` returns a collection of instance methods, so
   the collection is splatted to `module_function` to turn them
   into functions on the Feature module.

Signed-off-by: Robb Kidd <rkidd@chef.io>
@robbkidd robbkidd force-pushed the robb/super-forwardable-feature branch from e0120fb to 7822895 Compare October 13, 2017 19:03
@robbkidd
Copy link
Contributor Author

@sigje @jjasghar Added more commentary to both the commit message and a code comment. 👀

@robbkidd robbkidd merged commit ce43cb0 into master Oct 13, 2017
@robbkidd robbkidd deleted the robb/super-forwardable-feature branch October 13, 2017 19:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iennae iennae iennae approved these changes

@thommay thommay Awaiting requested review from thommay

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@robbkidd @jjasghar @iennae