Add new AttributeDefaultBlockValue cop

[cilim]

Hi everyone! 👋
Keep up the good work, we all love you for doing what you do 😃
This is my 1st contribution to this gem, so I hope it's not totally worthless, but we're planning to use it at my workplace since it seems like a good enough cop.

Since Rails 5 introduced the Attributes API more often than not I see this type of bug which can be hard to catch:

# bad
class User < ApplicationRecord
  attribute :confirmed_at, :datetime, default: Time.zone.now
end

# good
class User < ApplicationRecord
  attribute :confirmed_at, :datetime, default: -> { Time.zone.now }
end

This cop warns the developer if a method is passed to the default option without a block.

[andyw8]

I feel there are some situations where the default could be a fixed value and doesn't need to be evaluated in a block.

[cilim]

I feel there are some situations where the default could be a fixed value and doesn't need to be evaluated in a block.

Yup, there's definitely cases like that, in which the cop allows usage of static values:

class Post
  attribute :status, :string, default: :draft
end

[andyw8]

Sorry, I wasn't clear, I meant the default might be a method call whose return value doesn't change, e.g.:

class Post
  attribute :status, :string, default: SystemConfig.default_status
end

[cilim]

That is true @andyw8 👍

I personally think that the benefit of having this cop outweighs the disadvantage. So, it's basically a business decision :)

If you agree, let me know what's left to do. I see a failing spec regarding the documentation.

[andyw8]

Let's try get some further opinions before moving ahead.

[koic]

I see a failing spec regarding the documentation.

Con you mention it in the cop doc? I think it is helpful for users :-)

[cilim]

Thanks for reviewing @koic 😃

Just to let you know, I'll work on this sometimes next week, due to a project deadline.

[koic]

This looks good to me. I think this rule can also be added to the Rails Style Guide.
https://github.com/rubocop-hq/rails-style-guide

[cilim]

@koic we had an interesting case which is up for debate if we should add it to this cop.

For example, if you have something like this:

class Simple < ActiveType::Object
  attribute :mutable_attributes, default: {}
end

it might be used like so:

[1] pry(main)> first = Simple.new
=> #<Simple mutable_attributes: {}>
[2] pry(main)> first.mutable_attributes.merge!(a: 3)
=> {:a=>3}
[3] pry(main)> second = Simple.new
=> #<Simple mutable_attributes: {:a=>3}>
[4] pry(main)> second.mutable_attributes
=> {:a=>3}
[5] pry(main)> Simple.new.mutable_attributes.object_id
=> 70340537951880
[6] pry(main)> Simple.new.mutable_attributes.object_id
=> 70340537951880

as you see, the mutable_attributes is shared among instances, which you probably don't want.

The solution would be to just put the array in a block, which isn't difficult to add.

Same goes for array literals [].

Let me know how we should proceed. Open new issue, just push new commits here or something else.

[koic]

@cilim Thank you for the follow up. Can you open a new pull request? We will be able to discuss based on the description, test case, and implementation :-)

[cilim]

@cilim Thank you for the follow up. Can you open a new pull request? We will be able to discuss based on the description, test case, and implementation :-)

PR created :)

[jfelchner]

@koic just a heads up that this https://github.com/rubocop/rubocop-rails/pull/339/files#diff-3653213e3a153018a75cc7b2536b1d0217a6d8d75188d71cf45441812f789698R113 is incorrect and should be app/models