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 default field values #7

Merged
merged 7 commits into from Jul 24, 2019

Conversation

@dreikanter
Copy link
Contributor

commented Jul 18, 2019

Hey @palkan! I've noticed this post and decided to help with the implementation. It is a nice feature that I'd probably find a good use myself.

I've done quick research, and reproduced Attributes API behavior. Here is a quick demo:

class CreatePosts < ActiveRecord::Migration[5.2]
  def change
    create_table :posts do |t|
      t.json :settings, null: false, default: {}

      t.timestamps
    end
  end
end
Post.new.settings
=> {"color"=>"red"}
Post.new.color
=> "red"
Post.new(color: "banana").settings
=> {"color"=>"banana"}
Post.create!(color: 'banana')
   (0.2ms)  begin transaction
  Post Create (0.8ms)  INSERT INTO "posts" ("settings", "created_at", "updated_at") VALUES (?, ?, ?)  [["settings", "{\"color\":\"banana\"}"], ["created_at", "2019-07-18 17:49:09.112486"], ["updated_at", "2019-07-18 17:49:09.112486"]]
   (1.5ms)  commit transaction
=> #<Post id: 4, date: nil, settings: {"color"=>"banana"}, created_at: "2019-07-18 17:49:09", updated_at: "2019-07-18 17:49:09">
Post.new.save!
   (0.1ms)  begin transaction
  Post Create (0.4ms)  INSERT INTO "posts" ("created_at", "updated_at") VALUES (?, ?)  [["created_at", "2019-07-18 17:44:40.160795"], ["updated_at", "2019-07-18 17:44:40.160795"]]
   (2.0ms)  commit transaction
=> true

pp Post.last
  Post Load (0.1ms)  SELECT  "posts".* FROM "posts" ORDER BY "posts"."id" DESC LIMIT ?  [["LIMIT", 1]]
#<Post:0x00007fe878a0df58
 id: 2,
 date: nil,
 settings: {"color"=>"red"},
 created_at: Thu, 18 Jul 2019 17:44:40 UTC +00:00,
 updated_at: Thu, 18 Jul 2019 17:44:40 UTC +00:00>

Please take a look at the patch and let me know if it's necessary to update anything.

I have a question, though. Is it necessary to support dynamic (lambda) values for the default argument, or to follow the example from this comment? At the moment default support static values only.

PS: I've seen SumLare's comment in the #6 discussion. Please disregard my PR if the help is not necessary any longer. I had no intention to interfere if this is the case.

dreikanter added some commits Jul 18, 2019

@palkan

This comment has been minimized.

Copy link
Owner

commented Jul 18, 2019

Please take a look at the patch and let me know if it's necessary to update anything.

Thanks for your work! I'll take a look on Monday (feel free to ping me again If I do not).

Is it necessary to support dynamic (lambda) values for the default argument

Yes, if we want to make it work like the regular Attributes API. And we want)

I've seen SumLare's comment in the #6 discussion. Please disregard my PR if the help is not necessary any longer. I had no intention to interfere if this is the case.

There is no competition in open-source but there could be a cooperation. @SumLare feel free to jump in and share your thoughts on the feature and implementation (is it something similar to what you had in mind, and if not, what was your idea?).

Contributing != writing code, it also includes discussing and reviewing others code.

@SumLare

This comment has been minimized.

Copy link
Contributor

commented Jul 19, 2019

Sure, nicely done @dreikanter! I'm gonna put my hands on it tomorrow, may be I'll come with some ideas and help with that lambda default values

@SumLare

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2019

So I tried to follow with @dreikanter solution and implemented lambda values for defaults storing Proc in defaults hash(something like defaults["#{key}_proc"]) and then using .call to calculate it.

What I noticed is we basically have 3 places with almost the same condition which could live in one place. For me it would be something like this(untested code from my head):

def define_default_attribute(hash, key, type, caller)
  return unless defaults.key?(key)

  proc = defaults["#{key}_proc"]
  value = proc ? proc.call : defaults[key]

  case caller
  when :deserialize, :cast
    hash[key] = value
  when :serialize
    hash[key] = type.serialize(value)
  end
end

What do you think?

I couldn't think of a better way to handle default option so far, may be I'll come up with something better tomorrow since I'll have more time.

@dreikanter

This comment has been minimized.

Copy link
Contributor Author

commented Jul 20, 2019

@SumLare I think, it makes sense to use a stronger condition to distinguish a static default value from a lambda. Otherwise the user may define an attribute with _proc suffix and a non-lambda default value, and this code will try to call it. If I'm not missing something here.

As an alternative it is possible either to keep lambdas in a separate hash (like @dynamic_defaults), or just check if the default value is callable, and call it when necessary.

Regarding the conditions: yes, I've noticed this similarity too. Tried to refactor it, but didn't liked the result. So I kept it as is. This way it looks more transparent/readable. If you can make it dry, that would be nice, though.

I'll push an update to the original patch in a minute. It will also includes test cases for dynamic defaults, and some refactoring to reduce conditional code nesting.

@dreikanter dreikanter changed the title Support default field values #6 Support default field values Jul 20, 2019

@palkan
Copy link
Owner

left a comment

Great work 👍

Overall implementation looks good. Left a few comments.

lib/store_attribute/active_record/type/typed_store.rb Outdated Show resolved Hide resolved
lib/store_attribute/active_record/type/typed_store.rb Outdated Show resolved Hide resolved
lib/store_attribute/active_record/type/typed_store.rb Outdated Show resolved Hide resolved
lib/store_attribute/active_record/type/typed_store.rb Outdated Show resolved Hide resolved
lib/store_attribute/active_record/type/typed_store.rb Outdated Show resolved Hide resolved
spec/store_attribute/typed_store_spec.rb Show resolved Hide resolved
spec/support/user.rb Outdated Show resolved Hide resolved
@SumLare

This comment has been minimized.

Copy link
Contributor

commented Jul 22, 2019

Made some changes here

@palkan

palkan approved these changes Jul 23, 2019

Copy link
Owner

left a comment

LGTM 👍 Thanks!

One last thing: could you please update the Readme to reflect this change?

@SumLare

This comment has been minimized.

Copy link
Contributor

commented Jul 23, 2019

LGTM 👍 Thanks!

One last thing: could you please update the Readme to reflect this change?

Updated readme example here

@palkan palkan merged commit cf4d2f2 into palkan:master Jul 24, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@palkan

This comment has been minimized.

Copy link
Owner

commented Jul 24, 2019

Awesome! Thank you guys!

Could you please send me your contact info (email, twitter (if any)) and mailing address (we plan to send some goodies to CultOfMartians participants) to palkan@evl.ms?

@dreikanter

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2019

I was glad to participate :) Thank you @palkan and @SumLare! (I've just emailed my contact details.)

@palkan

This comment has been minimized.

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