Skip to content

Code review#2679

Merged
mshibuya merged 32 commits into
railsadminteam:masterfrom
HashNotAdam:hotfix/code_review
Jul 29, 2016
Merged

Code review#2679
mshibuya merged 32 commits into
railsadminteam:masterfrom
HashNotAdam:hotfix/code_review

Conversation

@HashNotAdam
Copy link
Copy Markdown

  • Create better conformity to the style guide.
  • Create compatibility with the Code Climate version of RuboCop to stop pull requests from being incorrectly labeled as failed.
  • Add RuboCop ignores for constants that are being used a mutable items since adjusting this behaviour would require a lot of testing.
  • DRY up some code seen in multiple places.

Adam Rice added 30 commits July 27, 2016 11:25
These methods are not aliases of each other and there are valid reasons for
sometimes desiring alias_method
# configuration.
register_instance_option :visible? do
associated_model_config.length > 0
!associated_model_config.empty?
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

!empty? == any?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I'll be honest with you... I didn't like this but also didn't take the time to look into what associated_model_config returned and so played it safe :)
Looking at it now, it seems like association.klass should always return an array so it should be safe to use any?

I'll make this change now.

@tiii
Copy link
Copy Markdown
Contributor

tiii commented Jul 28, 2016

👍

@mshibuya mshibuya merged commit eefa8ac into railsadminteam:master Jul 29, 2016
@mshibuya
Copy link
Copy Markdown
Member

Excellent work, thanks a lot!

@HashNotAdam
Copy link
Copy Markdown
Author

Wonderful, thank you, I'll rebase my other PRs now.

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