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

remove dynamic class creation where not needed #1850

Merged
merged 2 commits into from
Jul 18, 2016

Conversation

NullVoxPopuli
Copy link
Contributor

Purpose

Part n+1 in a long-running series of test improvements. This particular episode is to discourage people from (or at least not show them how to) dynamically creating classes for testing when they don't need to be dynamically created.

Changes

any class-level SubClass = Class.new(SuperClass) was converted to class SubClass < SuperClass; end

Caveats

None

Related GitHub issues

#1848 (comment)

Additional helpful information

Step next will be renaming tests from

def test_method_whatever to test 'this is what is being tested' do

@mention-bot
Copy link

@NullVoxPopuli, thanks for your PR! By analyzing the annotation information on this pull request, we identified @beauby, @joaomdmoura and @domitian to be potential reviewers

@bf4
Copy link
Member

bf4 commented Jul 18, 2016

@NullVoxPopuli looks good. Just two style issues:

Offenses:
test/fixtures/poro.rb:91:7: C: Style/ClassAndModuleChildren: Use nested module/class definitions instead of compact style.
class Spam::UnrelatedLink < Model; end
      ^^^^^^^^^^^^^^^^^^^
test/fixtures/poro.rb:279:7: C: Style/ClassAndModuleChildren: Use nested module/class definitions instead of compact style.
class Spam::UnrelatedLinkSerializer < ActiveModel::Serializer
      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

@NullVoxPopuli
Copy link
Contributor Author

@bf4 fixed. Thanks!

@bf4 bf4 added the S: LGTM label Jul 18, 2016
@bf4 bf4 merged commit aa4d89a into rails-api:master Jul 18, 2016
@bf4 bf4 deleted the tests-remove-dynamic-classes branch July 18, 2016 19:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants