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

Fix violations of and reenable rubocop Rails/Delegate. #14770

Merged
merged 3 commits into from May 2, 2017

Conversation

ashercodeorg
Copy link
Contributor

@ashercodeorg ashercodeorg commented May 1, 2017

Fixes initially generated automatically by bundle exec rubocop --only Rails/Delegate --auto-correct, with the changes to shared/ reverted by hand in 06c95c5.

Note that this PR enables the check only for the dashboard/ directory tree, as the rest of our codebase is not Rails.

@ashercodeorg
Copy link
Contributor Author

The failure seems legitimate.

230 tests, 779 assertions, 0 failures, 0 errors, 1 skips
Writing XML reports to /tmp/circle-junit.tEqRmXE/pegasus
[test] pegasus tests succeeded in 0:34 minutes
[test] Files affecting shared       tests *modified* from origin/staging. Starting tests. Changed files:
[test] 
    shared/test/files_api_test_base.rb
[test] Running shared tests...
RAILS_ENV=test RACK_ENV=test bundle exec rake test
Emptying /tmp/circle-junit.tEqRmXE/shared
/home/ubuntu/code-dot-org/shared/test/files_api_test_base.rb:84:in `<class:FilesApiTestBase>': undefined method `delegate' for FilesApiTestBase:Class (NoMethodError)
	from /home/ubuntu/code-dot-org/shared/test/files_api_test_base.rb:14:in `<top (required)>'
	from /home/ubuntu/code-dot-org/shared/test/test_animations.rb:1:in `require_relative'
	from /home/ubuntu/code-dot-org/shared/test/test_animations.rb:1:in `<top (required)>'
	from /home/ubuntu/code-dot-org/vendor/bundle/ruby/2.2.0/gems/rake-11.3.0/lib/rake/rake_test_loader.rb:10:in `require'
	from /home/ubuntu/code-dot-org/vendor/bundle/ruby/2.2.0/gems/rake-11.3.0/lib/rake/rake_test_loader.rb:10:in `block (2 levels) in <main>'
	from /home/ubuntu/code-dot-org/vendor/bundle/ruby/2.2.0/gems/rake-11.3.0/lib/rake/rake_test_loader.rb:9:in `each'
	from /home/ubuntu/code-dot-org/vendor/bundle/ruby/2.2.0/gems/rake-11.3.0/lib/rake/rake_test_loader.rb:9:in `block in <main>'
	from /home/ubuntu/code-dot-org/vendor/bundle/ruby/2.2.0/gems/rake-11.3.0/lib/rake/rake_test_loader.rb:4:in `select'
	from /home/ubuntu/code-dot-org/vendor/bundle/ruby/2.2.0/gems/rake-11.3.0/lib/rake/rake_test_loader.rb:4:in `<main>'
rake aborted!

@ashercodeorg
Copy link
Contributor Author

My belief is that this is because delegate is a Ruby on Rails method, not a Ruby method. And shared is not part of dashboard.

Does it make sense to enable the rubocop and disable the check within files_api_test_base.rb? Does it make sense to fix the "real" violation and blacklist the check?
Does some other approach make better sense?

@davidsbailey
Copy link
Member

Hey Asher, code in shared shouldn't have rails code in it, because it might be used in pegasus where rails isn't available. I would say that you could enable the check only for dashboard. Does that sound like a good option?

I don't think it makes sense to disable only for one file, and I'm not sure what you mean by 'fix the "real" violation'.

@davidsbailey
Copy link
Member

LGTM

@ashercodeorg
Copy link
Contributor Author

By "real" violation, I mean the violation in dashboard/lib/pd/payment/teacher_summary.rb. And I've updated the PR to only check the dashboard/ directory tree.

PTAL.

@ashercodeorg ashercodeorg merged commit 0dd214c into staging May 2, 2017
@ashercodeorg ashercodeorg deleted the rubocopRailsDelegate branch May 2, 2017 22:13
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.

None yet

2 participants