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 useless rescue #2647

Merged
merged 1 commit into from Aug 24, 2011
Merged

remove useless rescue #2647

merged 1 commit into from Aug 24, 2011

Conversation

dmathieu
Copy link
Contributor

params is a method, defined in every controller, which always returns a hash.
If it raises a NoMethodError, it means there's a bug somewhere else, which we want to know about.

rescue NoMethodError
false
end || Rails.application.config.assets.debug
params[:debug_assets] == '1' || params[:debug_assets] == 'true' || Rails.application.config.assets.debug
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you split this line up? Or perhaps use a more concise version, e.g.:

%w(1 true).include?(params[:debug_assets]) || Rails.application.config.assets.debug

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, that's something I can do. I don't like the %w format though (that's a very personal opinion) though.

Copy link
Contributor

Choose a reason for hiding this comment

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

['1', 'true'] is still an improvement. I'm on a crusade against long lines in Rails these days.

@spastorino
Copy link
Contributor

I prefer the first version is more readable and faster you can split the lines anyway ...

params[:debug_assets] == '1' ||
params[:debug_assets] == 'true' ||
Rails.application.config.assets.debug

params is a method, defined in every controller, which always returns a hash.
If it raises a NoMethodError, it means there's a bug somewhere else, which we want to know about.
@dmathieu
Copy link
Contributor Author

I've changed it. Thank you for your opinion :)

spastorino added a commit that referenced this pull request Aug 24, 2011
@spastorino spastorino merged commit f162f84 into rails:master Aug 24, 2011
spastorino added a commit that referenced this pull request Aug 24, 2011
spastorino added a commit that referenced this pull request Aug 24, 2011
spastorino added a commit that referenced this pull request Aug 30, 2011
spastorino added a commit that referenced this pull request Aug 30, 2011
spastorino added a commit that referenced this pull request Aug 30, 2011
tenderlove added a commit that referenced this pull request Aug 31, 2011
* 3-1-0:
  bumping to 3.1.0
  Bump sprockets up
  Depend on sass-rails and coffee-rails 3.1.0
  Revert "Merge pull request #2647 from dmathieu/no_rescue"
  Merge pull request #2756 from guilleiguaran/manifest-location
  Merge pull request #2748 from guilleiguaran/assets-version-config
  adds the asset pipeline guide to the index
  incorporate feedback from vijaydev and dasch to rephrase this to sound more natural, and some grammar fixes.
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

3 participants