search

rubymotion-community / motion-support

Commonly useful extensions to the standard library for RubyMotion
MIT License
132 stars 28 forks source link

Built In Type Extensions from MotionModel #2

Closed sxross closed 11 years ago

sxross commented 11 years ago

Added Hash#empty? and Hash#except Added inflector Added nilclass extensions Changed String#pluralize and String#singularize to use Inflector, added String#camelize, String#underscore, String#humanize, String#titleize, String#empty? and modified String#classify to return a real class instead of a string. Added symbol extensions Added array extensions Added specs for the foregoing

Note that there is a different behavior for String#classify. It now actually gets the class and returns an object of type Class.

tkadauke commented 11 years ago

This looks great! I love the Inflector :) A couple of things I noticed though:

  1. I just looked up the documentation of String#classify from ActiveSupport again. It behaves like the original version in motion-support. I think we should revert it to the original version and add a #constantize method to String. This way we don't violate the principle of least surprise.
  2. In the original ActiveSupport there is no NilClass#empty?. Thinking about it, nil.empty? does not make much sense: nil is not empty since it is no container. This is what blank? is for. I vote for removing it.
  3. Why is there a Symbol#titleize but none of the other methods defined for Symbol? I think this shortcut is not necessary and violates the Law of Demeter. I vote for removing it.
  4. There are a couple of specs missing.

What do you think? I'd be happy to help out with the above points.

sxross commented 11 years ago

On Mar 13, 2013, at 1:55 AM, Thomas Kadauke notifications@github.com wrote:

This looks great! I love the Inflector :) A couple of things I noticed though:

I just looked up the documentation of String#classify from ActiveSupport again. It behaves like the original version in motion-support. I think we should revert it to the original version and add a #constantize method to String. This way we don't violate the principle of least surprise.

I agree with the part about the POLS but it's surprising that Classify would return anything other than type Class. Perhaps this is a place where Rails got it wrong. Thoughts?

In the original ActiveSupport there is no NilClass#empty?. Thinking about it, nil.empty? does not make much sense: nil is not empty since it is no container. This is what blank? is for. I vote for removing it. You're right. I think blank? would be more like it. I'll look into this. Why is there a Symbol#titleize but none of the other methods defined for Symbol? I think this shortcut is not necessary and violates the Law of Demeter. I vote for removing it. The reason there is a Symbol#titleize is that it's common in code that emulates ActiveRecord or ActiveModel to pass around class name references as symbols. To turn these into labels for forms, titleize is useful. If you think this is a LOD violation, I'll be happy to remove it. There are a couple of specs missing. I can do this but need your input on the above before proceeding. What do you think? I'd be happy to help out with the above points.

— Reply to this email directly or view it on GitHub.

sxross commented 11 years ago

I believe my latest push addresses these issues. Please let me know.

Steve

On Mar 13, 2013, at 11:38 AM, Steve Ross sross@calicowebdev.com wrote:

On Mar 13, 2013, at 1:55 AM, Thomas Kadauke notifications@github.com wrote:

This looks great! I love the Inflector :) A couple of things I noticed though:

I just looked up the documentation of String#classify from ActiveSupport again. It behaves like the original version in motion-support. I think we should revert it to the original version and add a #constantize method to String. This way we don't violate the principle of least surprise.

I agree with the part about the POLS but it's surprising that Classify would return anything other than type Class. Perhaps this is a place where Rails got it wrong. Thoughts?

In the original ActiveSupport there is no NilClass#empty?. Thinking about it, nil.empty? does not make much sense: nil is not empty since it is no container. This is what blank? is for. I vote for removing it. You're right. I think blank? would be more like it. I'll look into this. Why is there a Symbol#titleize but none of the other methods defined for Symbol? I think this shortcut is not necessary and violates the Law of Demeter. I vote for removing it. The reason there is a Symbol#titleize is that it's common in code that emulates ActiveRecord or ActiveModel to pass around class name references as symbols. To turn these into labels for forms, titleize is useful. If you think this is a LOD violation, I'll be happy to remove it. There are a couple of specs missing. I can do this but need your input on the above before proceeding. What do you think? I'd be happy to help out with the above points.

— Reply to this email directly or view it on GitHub.

tkadauke commented 11 years ago

This looks wonderful. Thank you for that. The blank? methods were already present in blank.rb … I am not sure if my approach of dividing code was good. Now I tend to like your approach better. So I will accept your pull request as soon as I get a chance tomorrow (Berlin time: it's night here). Then I will remove my parts that are redundant now.

Thanks again for your contribution, it is very much appreciated! :-)