Re: [Rails-core] Re: Adding Model#model_name
Pull request sent: https://github.com/rails/rails/pull/15871 On Fri, Jun 20, 2014 at 7:15 AM, DHH da...@loudthinking.com wrote: I'm very much in favor of this as well. Please make it so. To address Andy's concerns, we should have proper error messages if this is overwriting an attribute of the same name. But it's too convenient not to have. On Friday, May 30, 2014 8:11:53 PM UTC+2, Jeremy Friesen wrote: Following up on https://github.com/rails/rails/issues/15428, with adjustments. Throughout the Rails code there are a few references to `object.class.model_name`. Instead of always pushing this up to the class, it makes sense to me that we ask the model for its model_name. This might obviate the need for the dubious ActionController::ModelNaming# model_name_from_record_or_class, https://github.com/rails/ rails/blob/master/actionpack/lib/action_controller/model_naming.rb module ActionController module ModelNaming # Converts the given object to an ActiveModel compliant one. def convert_to_model(object) object.respond_to?(:to_model) ? object.to_model : object end def model_name_from_record_or_class(record_or_class) (record_or_class.is_a?(Class) ? record_or_class : convert_to_model(record_or_class).class).model_name end end end Example: class Foo extend ActiveModel::Naming def model_name class.model_name endend Foo.respond_to?(:model_name)= true Foo.new.respond_to?(:model_name)= true Foo.model_name == Foo.new.model_name= true -- You received this message because you are subscribed to the Google Groups Ruby on Rails: Core group. To unsubscribe from this group and stop receiving emails from it, send an email to rubyonrails-core+unsubscr...@googlegroups.com. To post to this group, send email to rubyonrails-core@googlegroups.com. Visit this group at http://groups.google.com/group/rubyonrails-core. For more options, visit https://groups.google.com/d/optout. -- You received this message because you are subscribed to the Google Groups Ruby on Rails: Core group. To unsubscribe from this group and stop receiving emails from it, send an email to rubyonrails-core+unsubscr...@googlegroups.com. To post to this group, send email to rubyonrails-core@googlegroups.com. Visit this group at http://groups.google.com/group/rubyonrails-core. For more options, visit https://groups.google.com/d/optout.
[Rails-core] Feature Request: Do not allow to give Exception to rescue_from in controllers or at least show warning
When building dynamic error pages, people (specially beginners) always write rescue_from(Exception, ...) in ApplicationController. You can see it (or something similar) is suggested on Stackoverflow http://stackoverflow.com/questions/5331008/dynamic-error-pages-in-rails-3 and even RailsCasts http://railscasts.com/episodes/53-handling-exceptions-revised. However, doing so has serious side-effects such as listed below because it halts the error chain: * No longer able to see nice exception details during development * gems like exception_notification and airbrake stops working In addition, I've never seen any good use-cases of it in controllers. So I think it only gives us serious issues. And here's what I propose: * ActiveSupport::Rescuable remains the same * ActionController::Rescue overrides rescue_from and change it to not accept Exception(raise some exception or output warning) Since rescue_from is a class method, we can be notified when the app spins up or re-loads everything during development, not run time. What do you think? Yuki Nishijima -- You received this message because you are subscribed to the Google Groups Ruby on Rails: Core group. To unsubscribe from this group and stop receiving emails from it, send an email to rubyonrails-core+unsubscr...@googlegroups.com. To post to this group, send email to rubyonrails-core@googlegroups.com. Visit this group at http://groups.google.com/group/rubyonrails-core. For more options, visit https://groups.google.com/d/optout.
Re: [Rails-core] Re: Adding Model#model_name
Awesome you guys. 2014-06-23 13:42 GMT+00:00 Yuki Nishijima m...@yukinishijima.net: Pull request sent: https://github.com/rails/rails/pull/15871 On Fri, Jun 20, 2014 at 7:15 AM, DHH da...@loudthinking.com wrote: I'm very much in favor of this as well. Please make it so. To address Andy's concerns, we should have proper error messages if this is overwriting an attribute of the same name. But it's too convenient not to have. On Friday, May 30, 2014 8:11:53 PM UTC+2, Jeremy Friesen wrote: Following up on https://github.com/rails/rails/issues/15428, with adjustments. Throughout the Rails code there are a few references to `object.class.model_name`. Instead of always pushing this up to the class, it makes sense to me that we ask the model for its model_name. This might obviate the need for the dubious ActionController::ModelNaming#model_name_from_record_or_class, https://github.com/rails/rails/blob/master/actionpack/ lib/action_controller/model_naming.rb module ActionController module ModelNaming # Converts the given object to an ActiveModel compliant one. def convert_to_model(object) object.respond_to?(:to_model) ? object.to_model : object end def model_name_from_record_or_class(record_or_class) (record_or_class.is_a?(Class) ? record_or_class : convert_to_model(record_or_class).class).model_name end end end Example: class Foo extend ActiveModel::Naming def model_name class.model_name endend Foo.respond_to?(:model_name)= true Foo.new.respond_to?(:model_name)= true Foo.model_name == Foo.new.model_name= true -- You received this message because you are subscribed to the Google Groups Ruby on Rails: Core group. To unsubscribe from this group and stop receiving emails from it, send an email to rubyonrails-core+unsubscr...@googlegroups.com. To post to this group, send email to rubyonrails-core@googlegroups.com. Visit this group at http://groups.google.com/group/rubyonrails-core. For more options, visit https://groups.google.com/d/optout. -- You received this message because you are subscribed to the Google Groups Ruby on Rails: Core group. To unsubscribe from this group and stop receiving emails from it, send an email to rubyonrails-core+unsubscr...@googlegroups.com. To post to this group, send email to rubyonrails-core@googlegroups.com. Visit this group at http://groups.google.com/group/rubyonrails-core. For more options, visit https://groups.google.com/d/optout. -- You received this message because you are subscribed to the Google Groups Ruby on Rails: Core group. To unsubscribe from this group and stop receiving emails from it, send an email to rubyonrails-core+unsubscr...@googlegroups.com. To post to this group, send email to rubyonrails-core@googlegroups.com. Visit this group at http://groups.google.com/group/rubyonrails-core. For more options, visit https://groups.google.com/d/optout.
Re: [Rails-core] Feature Request: Do not allow to give Exception to rescue_from in controllers or at least show warning
On Jun 23, 2014, at 9:48 AM, Yuki Nishijima m...@yukinishijima.net wrote: When building dynamic error pages, people (specially beginners) always write rescue_from(Exception, ...) in ApplicationController. You can see it (or something similar) is suggested on Stackoverflow and even RailsCasts. The RailsCasts example is framed as “you really shouldn’t do this” with explanatory text as to *why* it’s a bad thing to do. The StackOverflow example is explicitly attempting to construct a general exception notifier - a *replacement* for exception_notification and airbrake. However, doing so has serious side-effects such as listed below because it halts the error chain: * No longer able to see nice exception details during development * gems like exception_notification and airbrake stops working In addition, I've never seen any good use-cases of it in controllers. So I think it only gives us serious issues. And here's what I propose: * ActiveSupport::Rescuable remains the same * ActionController::Rescue overrides rescue_from and change it to not accept Exception(raise some exception or output warning) Rescuing `Exception` is not the best practice, but there are reasons that somebody might want to do it. We can’t save people from doing silly-but-potentially-valid things. —Matt Jones signature.asc Description: Message signed with OpenPGP using GPGMail
Re: [Rails-core] Feature Request: Do not allow to give Exception to rescue_from in controllers or at least show warning
Rescuing `Exception` is not the best practice, but there are reasons that somebody might want to do it. We can’t save people from doing silly-but-potentially-valid things. I agree, but think a warning would be appropriate. -Amiel On Mon, Jun 23, 2014 at 4:03 PM, Matt Jones al2o...@gmail.com wrote: On Jun 23, 2014, at 9:48 AM, Yuki Nishijima m...@yukinishijima.net wrote: When building dynamic error pages, people (specially beginners) always write rescue_from(Exception, ...) in ApplicationController. You can see it (or something similar) is suggested on Stackoverflow http://stackoverflow.com/questions/5331008/dynamic-error-pages-in-rails-3 and even RailsCasts http://railscasts.com/episodes/53-handling-exceptions-revised. The RailsCasts example is framed as “you really shouldn’t do this” with explanatory text as to *why* it’s a bad thing to do. The StackOverflow example is explicitly attempting to construct a general exception notifier - a *replacement* for exception_notification and airbrake. However, doing so has serious side-effects such as listed below because it halts the error chain: * No longer able to see nice exception details during development * gems like exception_notification and airbrake stops working In addition, I've never seen any good use-cases of it in controllers. So I think it only gives us serious issues. And here's what I propose: * ActiveSupport::Rescuable remains the same * ActionController::Rescue overrides rescue_from and change it to not accept Exception(raise some exception or output warning) Rescuing `Exception` is not the best practice, but there are reasons that somebody might want to do it. We can’t save people from doing silly-but-potentially-valid things. —Matt Jones -- You received this message because you are subscribed to the Google Groups Ruby on Rails: Core group. To unsubscribe from this group and stop receiving emails from it, send an email to rubyonrails-core+unsubscr...@googlegroups.com. To post to this group, send email to rubyonrails-core@googlegroups.com. Visit this group at http://groups.google.com/group/rubyonrails-core. For more options, visit https://groups.google.com/d/optout.
Re: [Rails-core] Re: Adding Model#model_name
Excellent start Yuki! I started working on this as well on Friday and came to pretty much the same solution. I would have just left my changes in favor of Yuki's PR, but I think is misses the point. The idea is for rails to stop calling x.class.model_name, and only call x.model_name. I've started another PR making those changes: https://github.com/rails/rails/pull/15889 There's still two things to address: 1. I've left a few REVIEW comments. I'm looking for feedback there, but will remove them and rebase soon enough. 2. I'm still working on a warning to address the issue that Andy Jeffries brought up in this thread. I'm thinking it should just be a warning. I'm digging into it now; does anyone know of an example of rails currently doing this? Thanks! -Amiel On Mon, Jun 23, 2014 at 6:42 AM, Yuki Nishijima m...@yukinishijima.net wrote: Pull request sent: https://github.com/rails/rails/pull/15871 On Fri, Jun 20, 2014 at 7:15 AM, DHH da...@loudthinking.com wrote: I'm very much in favor of this as well. Please make it so. To address Andy's concerns, we should have proper error messages if this is overwriting an attribute of the same name. But it's too convenient not to have. On Friday, May 30, 2014 8:11:53 PM UTC+2, Jeremy Friesen wrote: Following up on https://github.com/rails/rails/issues/15428, with adjustments. Throughout the Rails code there are a few references to `object.class.model_name`. Instead of always pushing this up to the class, it makes sense to me that we ask the model for its model_name. This might obviate the need for the dubious ActionController::ModelNaming#model_name_from_record_or_class, https://github.com/rails/rails/blob/master/actionpack/ lib/action_controller/model_naming.rb module ActionController module ModelNaming # Converts the given object to an ActiveModel compliant one. def convert_to_model(object) object.respond_to?(:to_model) ? object.to_model : object end def model_name_from_record_or_class(record_or_class) (record_or_class.is_a?(Class) ? record_or_class : convert_to_model(record_or_class).class).model_name end end end Example: class Foo extend ActiveModel::Naming def model_name class.model_name endend Foo.respond_to?(:model_name)= true Foo.new.respond_to?(:model_name)= true Foo.model_name == Foo.new.model_name= true -- You received this message because you are subscribed to the Google Groups Ruby on Rails: Core group. To unsubscribe from this group and stop receiving emails from it, send an email to rubyonrails-core+unsubscr...@googlegroups.com. To post to this group, send email to rubyonrails-core@googlegroups.com. Visit this group at http://groups.google.com/group/rubyonrails-core. For more options, visit https://groups.google.com/d/optout. -- You received this message because you are subscribed to the Google Groups Ruby on Rails: Core group. To unsubscribe from this group and stop receiving emails from it, send an email to rubyonrails-core+unsubscr...@googlegroups.com. To post to this group, send email to rubyonrails-core@googlegroups.com. Visit this group at http://groups.google.com/group/rubyonrails-core. For more options, visit https://groups.google.com/d/optout. -- You received this message because you are subscribed to the Google Groups Ruby on Rails: Core group. To unsubscribe from this group and stop receiving emails from it, send an email to rubyonrails-core+unsubscr...@googlegroups.com. To post to this group, send email to rubyonrails-core@googlegroups.com. Visit this group at http://groups.google.com/group/rubyonrails-core. For more options, visit https://groups.google.com/d/optout.
Re: [Rails-core] Feature Request: Do not allow to give Exception to rescue_from in controllers or at least show warning
Make it a warning, and let people turn the warning off if they actually need it for some reason (though really, I can't think of a valid use case). Cheers, -foca On Mon, Jun 23, 2014 at 7:54 PM, Amiel Martin am...@carnesmedia.com wrote: Rescuing `Exception` is not the best practice, but there are reasons that somebody might want to do it. We can’t save people from doing silly-but-potentially-valid things. I agree, but think a warning would be appropriate. -Amiel On Mon, Jun 23, 2014 at 4:03 PM, Matt Jones al2o...@gmail.com wrote: On Jun 23, 2014, at 9:48 AM, Yuki Nishijima m...@yukinishijima.net wrote: When building dynamic error pages, people (specially beginners) always write rescue_from(Exception, ...) in ApplicationController. You can see it (or something similar) is suggested on Stackoverflow http://stackoverflow.com/questions/5331008/dynamic-error-pages-in-rails-3 and even RailsCasts http://railscasts.com/episodes/53-handling-exceptions-revised. The RailsCasts example is framed as “you really shouldn’t do this” with explanatory text as to *why* it’s a bad thing to do. The StackOverflow example is explicitly attempting to construct a general exception notifier - a *replacement* for exception_notification and airbrake. However, doing so has serious side-effects such as listed below because it halts the error chain: * No longer able to see nice exception details during development * gems like exception_notification and airbrake stops working In addition, I've never seen any good use-cases of it in controllers. So I think it only gives us serious issues. And here's what I propose: * ActiveSupport::Rescuable remains the same * ActionController::Rescue overrides rescue_from and change it to not accept Exception(raise some exception or output warning) Rescuing `Exception` is not the best practice, but there are reasons that somebody might want to do it. We can’t save people from doing silly-but-potentially-valid things. —Matt Jones -- You received this message because you are subscribed to the Google Groups Ruby on Rails: Core group. To unsubscribe from this group and stop receiving emails from it, send an email to rubyonrails-core+unsubscr...@googlegroups.com. To post to this group, send email to rubyonrails-core@googlegroups.com. Visit this group at http://groups.google.com/group/rubyonrails-core. For more options, visit https://groups.google.com/d/optout. -- You received this message because you are subscribed to the Google Groups Ruby on Rails: Core group. To unsubscribe from this group and stop receiving emails from it, send an email to rubyonrails-core+unsubscr...@googlegroups.com. To post to this group, send email to rubyonrails-core@googlegroups.com. Visit this group at http://groups.google.com/group/rubyonrails-core. For more options, visit https://groups.google.com/d/optout.
