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

Support proc as default value for Attributes API #19914

Closed
wants to merge 1 commit into from

Conversation

@kirs
Copy link
Member

commented Apr 27, 2015

Example:

attribute :year, :integer, default: -> { Time.now.year }

/cc @sgrif

@arthurnn

This comment has been minimized.

Copy link
Member

commented Apr 27, 2015

🆒 👍

@sgrif

This comment has been minimized.

Copy link
Contributor

commented Apr 27, 2015

Is there an equivalent of this possible prior to the attributes API?

@sgrif

This comment has been minimized.

Copy link
Contributor

commented Apr 27, 2015

It also occurs to me that this could interact with dirty checking in unexpected ways

@egilburg

This comment has been minimized.

Copy link
Contributor

commented Apr 27, 2015

Will this be evaluated in context of instance? Can you do something like this?

attribute :full_name, :string, default: -> { "#{first_name} #{last_name}" }

If not, would be nice to pass instance to the proc so one can do:

attribute :full_name, :string, default: -> { |instance| "#{instance.first_name} #{instance.last_name}" }
@kirs

This comment has been minimized.

Copy link
Member Author

commented Apr 27, 2015

@egilburg I was thinking about it, but now the proc is before other attributes are assigned. I will work on it.

Is there an equivalent of this possible prior to the attributes API?

@sgrif only possible using the before_validate callback.
Do you think we can merge if I add more tests concerning dirty checking?

@sgrif

This comment has been minimized.

Copy link
Contributor

commented Apr 27, 2015

I'm working on a different API to compose multiple values, I'd rather not overcomplicate it here by passing the instance. I'm not sure that the problem is too little test coverage, if we wrote a test for the edge case, it wouldnt be unexpected. ^_^

I think this is probably fine, I just want to be conservative about changes that could increase the complexity here.

@sgrif

This comment has been minimized.

Copy link
Contributor

commented Apr 27, 2015

The current implementation will call the proc once at definition time, not every time we need a default value.

@kirs

This comment has been minimized.

Copy link
Member Author

commented Apr 27, 2015

@sgrif fixed!

Support proc as default value for Attributes API
```
attribute :year, :integer, default: -> { Time.now.year }
```
@kirs

This comment has been minimized.

Copy link
Member Author

commented May 2, 2015

@sgrif updated.

@kirs

This comment has been minimized.

Copy link
Member Author

commented May 17, 2015

ping @sgrif

@sgrif sgrif closed this in a6e3cda May 28, 2015

@chancancode

This comment has been minimized.

Copy link
Member

commented May 30, 2015

In the context of an AR model, when would this proc be evaluated?

@printercu

This comment has been minimized.

Copy link
Contributor

commented May 30, 2015

Seems like it affects not only default values: user.name = -> { generate_name } will call the proc to get the value.

@printercu

This comment has been minimized.

Copy link
Contributor

commented May 30, 2015

I've missed, this one is not merged.

@sgrif

This comment has been minimized.

Copy link
Contributor

commented May 31, 2015

The merged version of this feature will only affect defaults, not assignment. That was a consideration in the implementation.

@printercu

This comment has been minimized.

Copy link
Contributor

commented May 31, 2015

Yeah, cool 👍

@chancancode

This comment has been minimized.

Copy link
Member

commented May 31, 2015

My question remains: at what point is the proc evaluated/resolved? Only once at init time? Only once on first access? Only once on save? Everytime when accessed?

This /seems/ important, but the answer is unclear to me from the proposal and the tests. It is not immediately obvious to me which of these is most "correct" (most useful) either. (1 is probably correct; 2 & 3 seems somewhat plausible but problematic in practice; pretty sure 4 be completely useless) Are there more background discussion about this?

I have a feeling that this is going to come back to bite us if we don't think it through. Personally I feel that I need to see more real use cases to understand why this is actually needed and how it will actually be used.

Even if we are immediately convinced once of the options are correct, we should probably add an "integration" test for it.

@chancancode

This comment has been minimized.

Copy link
Member

commented May 31, 2015

(and document it)

@tiegz

This comment has been minimized.

Copy link
Contributor

commented Jun 1, 2015

(@sgrif per your first question, there was a method that was removed in 3.2 -- I'm personally excited for this to be available again: https://github.com/rails/rails/blob/3-1-stable/activesupport/lib/active_support/core_ext/module/attr_accessor_with_default.rb#L22)

@sgrif

This comment has been minimized.

Copy link
Contributor

commented Jun 1, 2015

Yeah, that comment roughly sums up my initial reaction as well. The same can be accomplished with overriding initialize or an after_initialize hook. I think @chancancode raises some excellent points, that might be worth rethinking the feature. I'm pretty much nights and weekends right now so I'll give this some more thought on Saturday.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.