Skip to content

Simplify uses of defined?#3561

Merged
jdufresne merged 1 commit into
railsadminteam:masterfrom
jdufresne:defined
Dec 5, 2022
Merged

Simplify uses of defined?#3561
jdufresne merged 1 commit into
railsadminteam:masterfrom
jdufresne:defined

Conversation

@jdufresne

Copy link
Copy Markdown
Member

If an instance variable isn't defined it will always return nil.

If an instance variable isn't defined it will always return nil.
@coveralls

coveralls commented Nov 5, 2022

Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.07%) to 95.886% when pulling 0b4308b on jdufresne:defined into 2a40976 on railsadminteam:master.

@jdufresne jdufresne merged commit 2fa86c3 into railsadminteam:master Dec 5, 2022
@jdufresne jdufresne deleted the defined branch December 5, 2022 18:40
return @excluded if defined?(@excluded)

@excluded = !RailsAdmin::AbstractModel.all.collect(&:model_name).include?(abstract_model.try(:model_name))
@excluded ||= !RailsAdmin::AbstractModel.all.collect(&:model_name).include?(abstract_model.try(:model_name))

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Is the behaviour change here intended? This change removes the benefit of memoisation when the result is falsely.

Ref: http://gavinmiller.io/2013/advanced-memoization-in-ruby/

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good catch. No that is not intended. If you put up a PR I'll approve, otherwise I'll try to get to it later today.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Restored in #3587. Thanks for reporting.

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.

3 participants