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

Use squish rather than strip_heredoc #25668

Merged

Conversation

kamipo
Copy link
Member

@kamipo kamipo commented Jul 3, 2016

No description provided.

@rails-bot
Copy link

r? @eileencodes

(@rails-bot has picked a reviewer for you, use r? to override)

@kaspth
Copy link
Contributor

kaspth commented Jul 3, 2016

Thanks, but strip_heredoc is the preferred approach and this seems a cosmetic commit ❤️

@kaspth kaspth closed this Jul 3, 2016
@kamipo
Copy link
Member Author

kamipo commented Jul 3, 2016

Originally, this error message was written by \ in #25241. Not intended to include a break line.

        raise ActiveRecordError,
          "cannot touch on a new or destroyed record object. Consider using " \
          "persisted?, new_record?, or destroyed? before touching"

This PR fixes to inspect the error.

Before:

$ ARCONN=sqlite3 bundle exec ruby -w -Itest test/cases/touch_later_test.rb -n test_touch_laster_raise_if_non_persisted
/Users/kamipo/.rbenv/versions/2.3.1/lib/ruby/gems/2.3.0/gems/bundler-1.12.5/lib/bundler/rubygems_integration.rb:468: warning: method redefined; discarding old find_spec_for_exe
/Users/kamipo/.rbenv/versions/2.3.1/lib/ruby/site_ruby/2.3.0/rubygems.rb:241: warning: previous definition of find_spec_for_exe was here
Using sqlite3
Run options: -n test_touch_laster_raise_if_non_persisted --seed 17655

# Running:

#<ActiveRecord::ActiveRecordError: cannot touch on a new or destroyed record object. Consider using
persisted?, new_record?, or destroyed? before touching
>
.

Finished in 0.026475s, 37.7715 runs/s, 75.5430 assertions/s.

1 runs, 2 assertions, 0 failures, 0 errors, 0 skips

After:

$ ARCONN=sqlite3 bundle exec ruby -w -Itest test/cases/touch_later_test.rb -n test_touch_laster_raise_if_non_persisted
/Users/kamipo/.rbenv/versions/2.3.1/lib/ruby/gems/2.3.0/gems/bundler-1.12.5/lib/bundler/rubygems_integration.rb:468: warning: method redefined; discarding old find_spec_for_exe
/Users/kamipo/.rbenv/versions/2.3.1/lib/ruby/site_ruby/2.3.0/rubygems.rb:241: warning: previous definition of find_spec_for_exe was here
Using sqlite3
Run options: -n test_touch_laster_raise_if_non_persisted --seed 45541

# Running:

#<ActiveRecord::ActiveRecordError: cannot touch on a new or destroyed record object. Consider using persisted?, new_record?, or destroyed? before touching>
.

Finished in 0.026327s, 37.9838 runs/s, 75.9676 assertions/s.

1 runs, 2 assertions, 0 failures, 0 errors, 0 skips

@kamipo
Copy link
Member Author

kamipo commented Jul 3, 2016

--- a/activerecord/test/cases/touch_later_test.rb
+++ b/activerecord/test/cases/touch_later_test.rb
@@ -12,9 +12,10 @@ def test_touch_laster_raise_if_non_persisted
     invoice = Invoice.new
     Invoice.transaction do
       assert_not invoice.persisted?
-      assert_raises(ActiveRecord::ActiveRecordError) do
+      e = assert_raises(ActiveRecord::ActiveRecordError) do
         invoice.touch_later
       end
+      p e
     end
   end

@kamipo
Copy link
Member Author

kamipo commented Jul 3, 2016

https://github.com/rails/rails/blob/v5.0.0/activerecord/lib/active_record/reflection.rb#L409-L413

          raise ArgumentError, <<-MSG.squish
            The association scope '#{name}' is instance dependent (the scope
            block takes an argument). Preloading instance dependent scopes is
            not supported.
          MSG

@kaspth
Copy link
Contributor

kaspth commented Jul 3, 2016

Lots of our other error messages use strip_heredoc and contain line breaks. I don't understand what makes these so different?

@matthewd
Copy link
Member

matthewd commented Jul 3, 2016

@kaspth got some examples? I only see a few, all of which are using the line breaks for structure, rather than word wrapping.

OTOH, I don't like this message anyway, so... 😕

@kamipo
Copy link
Member Author

kamipo commented Jul 5, 2016

See also #23838.

@rafaelfranca rafaelfranca reopened this Jul 5, 2016
@rafaelfranca rafaelfranca merged commit 2797de2 into rails:master Jul 5, 2016
rafaelfranca added a commit that referenced this pull request Jul 5, 2016
…redoc

Use `squish` rather than `strip_heredoc`
@rafaelfranca
Copy link
Member

Backported in 6c0fd9f

@kamipo kamipo deleted the use_squish_rather_than_strip_heredoc branch July 5, 2016 18:05
@kaspth
Copy link
Contributor

kaspth commented Jul 5, 2016

@matthewd right you are, that was used for structure.

All good here 👍

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

7 participants