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

Attempt to look up non-existing settings in parent classes (see #21) #55

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

britishtea
Copy link

This fixes an issue where settings are added to Cuba.settings after inheriting from Cuba are not picked up by the child classes. The problem is described in issue #21.

If a setting does not exist, settings are looked up in the settings Hash of the parent class using Hash#default_proc. The settings Hash is still deepcloned, so settings are still overridable.

…an#21)

This fixes an issue where settings are added to Cuba.settings *after* inheriting
from `Cuba` are not picked up by the child classes. The problem is described in
issue soveran#21.

If a setting does not exist, Settings are looked up in the settings Hash of the parent class using `Hash#default_proc`. The settings Hash is still deepcloned, so
settings are still overridable.
@@ -86,11 +86,26 @@ def self.settings
end

def self.deepclone(obj)
Marshal.load(Marshal.dump(obj))
# Hashes with a default_proc cannot be serialized by Marshal.dump.
if obj.respond_to?(:default_proc)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious. Why the respond_to??

{}.respond_to?(:default_proc) # => true

Copy link
Author

Choose a reason for hiding this comment

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

I took a clue from the argument name (obj). Because Cuba.deepclone is part of the public API, this patch could break code that calls Cuba.deepclone with anything other than a Hash.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice. Thanks for replying :-)

@mkristian
Copy link

I personally solved this issue without deep cloning via Marshal since have two problems with Marshal

  • does not work with object with singleton methods defined
  • using jruby I can use java object along ruby object but they do not Marshal

I am using this cuba-plugin https://github.com/mkristian/cuba-api/blob/master/lib/cuba_api/config.rb
which replaces Cuba.settings['x'] with Cuba['x'] which behaves like settings but fixes the two above problems. sorry that I never attempt to bring this in as PR to cuba itself.

@soveran
Copy link
Owner

soveran commented Jan 15, 2016

I added a comment, please let me know what you guys think: #21 (comment)

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.

4 participants