search

rubymotion-community / motion-support

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

String extensions should be on NSString #6

Closed clayallsopp closed 11 years ago

clayallsopp commented 11 years ago

Looks like all of the string extensions are done to String, but I think it should be done to NSString?

A lot of times when objects are deserialized via Cocoa (i.e. NSUserDefaults), strings are made as instance sof NSString, which means these methods won't be found.

tkadauke commented 11 years ago

I agree. I didn't do it yet, since I didn't need it so far. There are basically two different ways to do that. One, like you said, add everything to NSString/NSMutableString. Two, leave things in String, and add a to_s method to NSString, giving the responsibility back to the user. Two point five would be to also add all the methods to NSString/NSMutableString, but delegate the calls to to_s. An example of the last one can be found in motion/core_ext/ns_dictionary.rb

What do you think?

clayallsopp commented 11 years ago

Hmmm, I don't like just using to_s because it can lead to a lot of unexpected behavior (i.e. you create a User object with String fields, serialize it, deserialize it, everything is now an NSString and you have to remember to coerce the fields back with to_s at some point). Would lead to a lot of surprises just because of a technical detail in how RubyMotion is implemented, and folks without a Cocoa background won't immediately understand why. I can already see the Github Issues popping up :)

But the delegation scheme could work...I guess the drawback is when new extensions are added, you have to remember to also add a corresponding method to the right core_ext file? Instead of just writing it in one place and being done with it. And that's not necessarily a bad thing, since new extensions won't be super common; just trying to make sure that's the only downside.

So yeah tl;dr either solution 1 or solution 2.5 sound good

tkadauke commented 11 years ago

Check out my commit in branch ns_string. This has the additional advantage that whenever you use an extension to class String that returns a different string, you're guaranteed to end up with a Ruby String instance instead of a NSString instance. What do you think?

clayallsopp commented 11 years ago

Very nice :+1:

tkadauke commented 11 years ago

Pushed the changes to master

clayallsopp commented 11 years ago

Hmmm, I think there's a bug in the implementation for multiple-argument methods:

(main)> "camel_case".camelize(true)
=> nil
(main)> "camel_case".camelize(false)
=> nil

# vs.
(main)> MotionSupport::Inflector.camelize("camel_case", true)
=> "CamelCase"
(main)> MotionSupport::Inflector.camelize("camel_case", false)
=> "camelCase"
tkadauke commented 11 years ago

Hmm I agree it is unexpected, but it's just like in ActiveRecord. It should be

"camel_case".camelize(:lower) "camel_case".camelize(:upper)

clayallsopp commented 11 years ago

Ahhh gotcha, makes sense.