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

Allow Rails/ReflectionClassName to use symbol argument #6791

Merged

Conversation

unasuke
Copy link
Contributor

@unasuke unasuke commented Feb 25, 2019

belongs_to :account, class_name: :Account

Rails/ReflectionClassName ( introduced from #6704 ) offences that code, but symbol is also uses as string so I think this code has no offences by the cop.


Before submitting the PR make sure the following are checked:

  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
  • The PR relates to only one subject with a clear title
    and description in grammatically correct, complete sentences.
  • Run bundle exec rake default. It executes all tests and RuboCop for itself, and generates the documentation.

@koic
Copy link
Member

koic commented Feb 25, 2019

That makes sense. Can you write the CHANGELOG entry?

@unasuke
Copy link
Contributor Author

unasuke commented Feb 25, 2019

@koic I see!

@unasuke unasuke force-pushed the rails_reflection_class_name_allow_symbol branch from 934261e to 84b1012 Compare February 25, 2019 08:14
CHANGELOG.md Outdated Show resolved Hide resolved
@unasuke unasuke force-pushed the rails_reflection_class_name_allow_symbol branch from 84b1012 to 7443b0e Compare February 25, 2019 08:21
@koic
Copy link
Member

koic commented Feb 25, 2019

Unfortunately I had overlooked the discussion below 💦
#6704 (comment)

I have an opportunity to meet Rails committer soon. I'd like to make a final judgment by listening to opinion as Rails. Please wait for a little while longer.

@koic koic self-assigned this Feb 25, 2019
@Drenmi
Copy link
Collaborator

Drenmi commented Feb 25, 2019

Looks like a number of different test cases have been squashed into one (even before you made this change.) Could you please separate the test cases so we test one thing per test? 🙂

@koic
Copy link
Member

koic commented Feb 25, 2019

Correct 💦 The test case of String object and the test case of Symbol object should have been independent.

@mikegee
Copy link
Contributor

mikegee commented Feb 25, 2019

I disagree with allowing a symbol here. It is immediately converted to a string with to_s, so we are ok requiring a string in the first place.

@koic
Copy link
Member

koic commented Mar 1, 2019

I think that using String for class_name will be more unified than using Symbol. However I'd like to accept this change. Because the following test case that accepts Symbol with class_name is written in test cases of Rails.
https://github.com/rails/rails/blob/v6.0.0.beta2/activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb#L950-L955

On the other hand, If you propose to Rails that it does not accept Symbol with class_name and accept it, let's change it so that Rails/ReflectionClassName cop does not accept Symbol at that time.

Cc @amatsuda @kamipo @y-yagi

@unasuke unasuke force-pushed the rails_reflection_class_name_allow_symbol branch 2 times, most recently from 7126d0d to af75e6e Compare March 13, 2019 09:47
@unasuke unasuke force-pushed the rails_reflection_class_name_allow_symbol branch from af75e6e to 9ab9534 Compare March 13, 2019 10:05
@koic koic merged commit a63d1cc into rubocop:master Mar 14, 2019
@koic
Copy link
Member

koic commented Mar 14, 2019

Thanks!

@unasuke
Copy link
Contributor Author

unasuke commented Mar 15, 2019

Thank you too!

@unasuke unasuke deleted the rails_reflection_class_name_allow_symbol branch March 15, 2019 04:55
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.

None yet

4 participants