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

Set up config_accessor with a default value by block #7645

Merged
merged 1 commit into from Sep 17, 2012

Conversation

larrylv
Copy link
Contributor

@larrylv larrylv commented Sep 14, 2012

  • ActiveSupport::Configurable should allow config_accessor to take
    default value by block, just like cattr_accessor.

      class User
        include ActiveSupport::Configurable
        config_accessor :hair_colors do
          [:brown, :black, :blonde, :red]
        end
      end
    
      User.hair_colors # => [:brown, :black, :blonde, :red]
    
  • remove trailing whitespaces in configurable.rb and its test file.

It's an old pull request #5730, but no one implement it . I just implement it like someone suggested, change the interface to just like cattr_accessor.

@larrylv
Copy link
Contributor Author

larrylv commented Sep 15, 2012

cc @steveklabnik @schneems

@carlosantoniodasilva
Copy link
Member

It seems ok to me, although I don't see a real gain from one over another:

config_accessor :hair_colors do
  [:brown, :black, :blonde, :red]
end

# vs

config_accessor :hair_colors
self.hair_colors = [:brown, :black, :blonde, :red]

In any case, your pull request needs a changelog entry as well. Thanks!

@larrylv
Copy link
Contributor Author

larrylv commented Sep 16, 2012

@carlosantoniodasilva I have updated ActiveSuppport's CHANGELOG.

@robin850
Copy link
Member

@larrylv : just rebase your commits squashing them into a single one. :)

@larrylv
Copy link
Contributor Author

larrylv commented Sep 16, 2012

@robin850 then do I have to open another pull request or just git push -f?

@robin850
Copy link
Member

@larrylv : once your commits are squashed, just push with -f. :) You don't have to open a new pull request, it will be automatically updated.

@larrylv
Copy link
Contributor Author

larrylv commented Sep 16, 2012

@robin850 Thx. Already done that. :-)

@robin850
Copy link
Member

I allow myself to ping @carlosantoniodasilva to merge this. ^^

@carlosantoniodasilva
Copy link
Member

@larrylv thanks, but I'll have to ask you to do one more thing: can you amend your commit and move the changelog entry to the top, please? We usually put them on the top. Thanks!

@robin850 thanks!

@larrylv
Copy link
Contributor Author

larrylv commented Sep 16, 2012

@carlosantoniodasilva Sorry, my mistake. Already moved changelog entry to the top.

@carlosantoniodasilva
Copy link
Member

Great! Now github is telling me: This pull request cannot be automatically merged.. I think you'll need to rebase :)

* ActiveSupport::Configurable should allow config_accessor to take
  default value by block, just like cattr_accessor.

    class User
      include ActiveSupport::Configurable
      config_accessor :hair_colors do
        [:brown, :black, :blonde, :red]
      end
    end

    User.hair_colors # => [:brown, :black, :blonde, :red]

* remove trailing whitespaces in configurable.rb and its test file.

* Update ActiveSupport CHANGELOG.
@larrylv
Copy link
Contributor Author

larrylv commented Sep 17, 2012

@carlosantoniodasilva Done with rebase. Please check it out. :)

rafaelfranca added a commit that referenced this pull request Sep 17, 2012
Set up config_accessor with a default value by block
@rafaelfranca rafaelfranca merged commit eac9e03 into rails:master Sep 17, 2012
@rafaelfranca
Copy link
Member

Thanks.

@zires
Copy link
Contributor

zires commented Sep 19, 2012

I am curious about why use block to define default value.The default option can take care of this, like:

config_accessor :hair_colors, default: [:brown, :black, :blonde, :red]

@larrylv
Copy link
Contributor Author

larrylv commented Sep 19, 2012

@zires I don't think config_accessor supports default option. U can check the config_accessor method here https://github.com/rails/rails/blob/master/activesupport/lib/active_support/configurable.rb#L106 . It doesn't do anything about default. And also I write test codes below to double check.

require 'active_support'

person = Class.new do
    include ActiveSupport::Configurable
    config_accessor :hair_colors, default: [:brown, :black, :blonde]
end

person.hair_colors # => nil

And the reason I choose to use block to define default value is cattr_accessor uses block to define it. U can check it here. https://github.com/rails/rails/blob/master/activesupport/lib/active_support/core_ext/class/attribute_accessors.rb#L32

In some way, they are very alike.

@bobbytables
Copy link

I think a block to always define a default value is weird (and more expensive). Why not have a default: key?

@zires
Copy link
Contributor

zires commented Oct 19, 2012

@bobbytables That is what i mean

@larrylv larrylv deleted the configurable-defaults branch November 11, 2014 03:38
glaszig added a commit to glaszig/blogit that referenced this pull request Mar 18, 2017
AS::Configurable's `config_accessor` supports a default block
only since rails 4 -- see rails/rails#7645.
glaszig added a commit to glaszig/blogit that referenced this pull request Mar 19, 2017
AS::Configurable's `config_accessor` supports a default block
only since rails 4 -- see rails/rails#7645.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants