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

Deprecate constant lookups via .const_missing #10

Merged
merged 17 commits into from
Aug 16, 2021
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ jobs:
runs-on: ubuntu-latest
strategy:
matrix:
rubyversion: ['2.4', '2.5', '2.6', '2.7', '3.0']
rubyversion: ['2.5', '2.6', '2.7']
lanej marked this conversation as resolved.
Show resolved Hide resolved
steps:
- uses: actions/checkout@v2
- name: set up ruby
Expand Down
5 changes: 5 additions & 0 deletions Gemfile
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
source 'https://rubygems.org'

gemspec

group :test do
gem "pry"
gem "pry-nav"
end
15 changes: 6 additions & 9 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,6 @@ features
└── test.yml
```

The classes `Feature::Test`, `Feature::Thing::One` and `Feature::Thing::Two` will be available for use within
your application.

You can call the `Toggles.init` method to force re-parsing the configuration and re-initializing all Features
structures at any time. The `Toggles.reinit_if_necessary` method is a convenience helper which will only
re-initialize of the top-level features directory has changed. Note that, in general, this will only detect
Expand All @@ -65,13 +62,13 @@ user:
Check if the feature is enabled or disabled:

```ruby
Feature::NewFeature::AvailableForPresentation.enabled_for?(user: OpenStruct.new(id: 12345)) # true
Feature::NewFeature::AvailableForPresentation.enabled_for?(user: OpenStruct.new(id: 54321)) # true
Feature::NewFeature::AvailableForPresentation.enabled_for?(user: OpenStruct.new(id: 7)) # false
Feature.enabled?(:new_feature, :available_for_presentation, user: OpenStruct.new(id: 12345)) # true
Feature.enabled?(:new_feature, :available_for_presentation, user: OpenStruct.new(id: 54321)) # true
Feature.enabled?(:new_feature, :available_for_presentation, user: OpenStruct.new(id: 7)) # false

Feature::NewFeature::AvailableForPresentation.disabled_for?(user: OpenStruct.new(id: 12345)) # false
Feature::NewFeature::AvailableForPresentation.disabled_for?(user: OpenStruct.new(id: 54321)) # false
Feature::NewFeature::AvailableForPresentation.disabled_for?(user: OpenStruct.new(id: 7)) # true
Feature.disabled?(:new_feature, :available_for_presentation, user: OpenStruct.new(id: 12345)) # false
Feature.disabled?(:new_feature, :available_for_presentation, user: OpenStruct.new(id: 54321)) # false
Feature.disabled?(:new_feature, :available_for_presentation, user: OpenStruct.new(id: 7)) # true
```

## License
Expand Down
3 changes: 3 additions & 0 deletions features/nested_foo/bar_baz.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
id:
lt:
5
52 changes: 7 additions & 45 deletions lib/toggles.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,63 +19,25 @@ def configuration
@configuration ||= Configuration.new
end

# Dynamically create modules and classes within the `Feature` module based on
# the directory structure of `features`.
#
# For example if the `features` directory has the structure:
#
# features
# ├── thing
# | ├── one.yml
# | └── two.yml
# └── test.yml
#
# `Feature::Test`, `Feature::Thing::One`, `Feature::Thing::Two` would be
# available by default.
#
def init
return unless Dir.exists? configuration.features_dir

new_tree = Module.new

top_level = File.realpath(configuration.features_dir)
top_level_p = Pathname.new(top_level)

Find.find(top_level) do |path|
previous = new_tree
abspath = path
path = Pathname.new(path).relative_path_from(top_level_p).to_s
if path.match(/\.ya?ml\Z/)
base = path.chomp(File.extname(path)).split("/")
if base.size > 1
directories = base[0...-1]
filename = base[-1]
else
directories = []
filename = base[0]
end

directories.each do |directory|
module_name = directory.split("_").map(&:capitalize).join.to_sym
previous = if previous.constants.include? module_name
previous.const_get(module_name)
else
previous.const_set(module_name, Module.new)
end
end
Feature.features.clear

cls = Class.new(Feature::Base) do |c|
c.const_set(:PERMISSIONS, Feature::Permissions.new(abspath))
end

previous.const_set(filename.split("_").map(&:capitalize).join.to_sym, cls)
Dir[File.join(top_level, "**/*.{yaml,yml}")].each do |abspath|
path = Pathname.new(abspath).relative_path_from(top_level_p).to_s
features = path.split("/")[0..-2].inject(Feature.features) { |a,e| a[e.to_sym] ||= {} }
feature_key = File.basename(path, File.extname(path)).to_sym
features[feature_key] = Class.new(Feature::Base) do |c|
c.const_set(:PERMISSIONS, Feature::Permissions.new(abspath))
end
end

stbuf = File.stat(top_level)
@stat_tuple = StatResult.new(stbuf.ino, stbuf.mtime)

Feature.set_tree(new_tree)
end

def reinit_if_changed
Expand Down
47 changes: 47 additions & 0 deletions lib/toggles/constant_lookup.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
class Feature::ConstantLookup
Error = Class.new(NameError) do
Copy link
Contributor

Choose a reason for hiding this comment

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

why not just define this as, you know,

class Error < NameError do
   ...
end

Anonymous classes often feel like a code smell to me

Copy link
Member Author

Choose a reason for hiding this comment

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

not an anonymous class, just a different way to declare a subclass.

attr_reader :sym

def initialize(sym)
@sym = sym
super(sym.join('::'))
end
end


# Return a tree walker that translates Module#const_missing(sym) into the next child node
#
# So Features::Cat::BearDog walks as:
# * next_features = Feature.features # root
# * const_missing(:Cat) => next_features = next_features['cat']
# * const_missing(:BearDog) => next_features['bear_dog']
#
# Defined at Toggles.features_dir + "/cat/bear_dog.yaml"
#
# @raise [Error] if constant cannot be resolved
def self.from(features, path)
Class.new {
class << self
attr_accessor :features
attr_accessor :path

def const_missing(sym)
subtree_or_feature = features.fetch(
# translate class name into path part i.e :BearDog #=> 'bear_dog'
sym.to_s.gsub(/([a-z])([A-Z])/) { |s| s.chars.join('_') }.downcase.to_sym,
)
if subtree_or_feature.is_a?(Hash)
Feature::ConstantLookup.from(subtree_or_feature, path + [sym])
else
subtree_or_feature
end
rescue KeyError
raise Error.new(path + [sym])
end
end
}.tap do |resolver|
resolver.features = features
resolver.path = path
end
lanej marked this conversation as resolved.
Show resolved Hide resolved
end
end
24 changes: 20 additions & 4 deletions lib/toggles/feature.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,29 @@ module Feature
or: Operation::Or,
range: Operation::Range}

@@tree = Module.new
Error = Class.new(StandardError)
Unknown = Class.new(Error)
Comment on lines +15 to +16
Copy link
Contributor

Choose a reason for hiding this comment

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

same question


def self.set_tree(tree)
@@tree = tree
def self.features
@features ||= {}
end

# @deprecated This is an abuse of lazy dispatch that creates cryptic errors
def self.const_missing(sym)
@@tree.const_get(sym, inherit: false)
ConstantLookup.from(features, [:Feature]).const_missing(sym)
end

def self.enabled?(*sym, **criteria)
sym
.inject(features) { |a, e| a.fetch(e) }
.enabled_for?(criteria)
rescue KeyError
raise Unknown, sym.inspect
end

def self.disabled?(*sym, **criteria)
!enabled?(*sym, **criteria)
end
end

require 'toggles/constant_lookup'
2 changes: 2 additions & 0 deletions spec/spec_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@

require "toggles"

Bundler.require(:test)

RSpec.configure do |config|
config.order = "random"

Expand Down
22 changes: 19 additions & 3 deletions spec/toggles/feature/acceptance/type_spec.rb
Original file line number Diff line number Diff line change
@@ -1,7 +1,23 @@
describe "Feature::Type" do
specify 'deprecated' do
aggregate_failures do
expect(Feature::Type.enabled_for?(user_id: 1)).to eq true
expect(Feature::Type.enabled_for?(user_id: 25)).to eq false
expect(Feature::Type.enabled_for?(user_id: nil)).to eq false
end
end

specify do
expect(Feature::Type.enabled_for?(user_id: 1)).to eq true
expect(Feature::Type.enabled_for?(user_id: 25)).to eq false
expect(Feature::Type.enabled_for?(user_id: nil)).to eq false
aggregate_failures do
expect(Feature.enabled?(:type, user_id: 1)).to eq(true)
expect(Feature.disabled?(:type, user_id: 1)).to eq(false)
expect(Feature.enabled?(:type, user_id: 25)).to eq(false)
expect(Feature.enabled?(:nested_foo, :bar_baz, id: 25)).to eq(false)
expect(Feature.enabled?(:nested_foo, :bar_baz, id: 1)).to eq(true)
expect(Feature.disabled?(:type, user_id: 25)).to eq(true)
expect(Feature.disabled?(:nested_foo, :bar_baz, id: 25)).to eq(true)
expect(Feature.disabled?(:nested_foo, :bar_baz, id: 1)).to eq(false)
expect { Feature.disabled?(:nested_foo, :bar_boz, id: 1) }.to raise_error(Feature::Unknown)
end
end
end
9 changes: 6 additions & 3 deletions spec/toggles/init_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
Toggles.configure do |c|
c.features_dir = temp_dir
end

expect(Feature::Foo::Users.enabled_for?(id: 1)).to eq(true)
expect(Feature::Bar::Users.enabled_for?(id: 3)).to eq(true)
end
Expand All @@ -36,7 +36,7 @@
Toggles.configure do |c|
c.features_dir = temp_dir
end

expect(Feature::Foo::Users.enabled_for?(id: 1)).to eq(true)
expect(Feature::Foo::Children.enabled_for?(id: 1)).to eq(true)

Expand All @@ -49,7 +49,10 @@
Toggles.init

expect(Feature::Foo::Users.enabled_for?(id: 1)).to eq(false)
expect { Feature::Foo::Children.enabled_for?(id: 1) }.to raise_error(NameError)
expect { Feature::Bar::Children.enabled_for?(id: 1) }
.to raise_error(Feature::ConstantLookup::Error, 'Feature::Bar')
expect { Feature::Foo::Children.enabled_for?(id: 1) }
.to raise_error(Feature::ConstantLookup::Error, 'Feature::Foo::Children')
lanej marked this conversation as resolved.
Show resolved Hide resolved
end
end

Expand Down
3 changes: 0 additions & 3 deletions toggles.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,6 @@ Gem::Specification.new do |s|
EOF

s.add_development_dependency "bundler"
s.add_development_dependency "pry"
s.add_development_dependency "pry-nav"
s.add_development_dependency "pry-remote"
s.add_development_dependency "rake"
s.add_development_dependency "rspec"
s.add_development_dependency "rspec-its"
Expand Down