rubymotion-community / BubbleWrap

Cocoa wrappers and helpers for RubyMotion (Ruby for iOS and OS X) - Making Cocoa APIs more Ruby like, one API at a time. Fork away and send your pull requests
Other
1.18k stars 208 forks source link

Remove instantiating Device as BW::Device #355

Open rantrix opened 10 years ago

rantrix commented 10 years ago

I'd like to suggest that we remove the convenience of instantiating Device as BW::Device if it is not defined.

The reason why is that this causes a name spacing problem that forces you to handle your load paths, which can cause a lot of headaches. Since BubbleWrap has already been shorten to BW, typing BW::Device isn't too far from typing Device.

It will also prevent other gems that rely on BW to explicitly call BW::Device instead of assuming that Device is BW::Device.

What do you think? I'd love to hear your thoughts on it.

Thank you!

clayallsopp commented 10 years ago

I don't have a strong opinion either way, so pretty open to hear others thoughts (@markrickert @colinta etc) but two thoughts:

markrickert commented 10 years ago

I use Device.simulator? all the time to have things behave differently in the simulator for testing (things like timeouts, etc). I suppose it wouldn't hurt to change those all to BW::Device.simulator?, but I think if it is done to Device we should also commit to doing it to App. BW::App.name doesn't seem as clean to me as App.name though

@DexterV Can you provide an example of a gem that has a namespacing problem with the Device class?

colinta commented 10 years ago

I think this is a sensible request, but we'll need an easy way to include the existing shorthands, so people can migrate easily. For instance a require 'bubble-wrap-polluting' or something like that.

I'm of the opinion that BubbleWrap should try and remain non-polluting, I always liked that it wrapped APIs and did not change the objects directly (like SugarCube does). I see this as a smart step in that direction.

rantrix commented 10 years ago

@clayallsopp I feel the same for the App shorthand. I think it's a good idea to be consistent and apply this to both Device and App. I've updated the pull request accordingly.

@markrickert I ran into the namespacing problem with Device while using Formotion. Clay also referenced the pull request for convenience.

@colinta I'm definitely on board.

I think giving the users the ability to namespace these classes themselves is a more friendly approach. It will also help prevent any management of loading paths in case there is a conflict.

Thank you everyone for taking the time to consider this suggestion.

clayallsopp commented 10 years ago

@colinta fwiw I don't find Device and App polluting in the same way that extending NSString etc is polluting

@DexterV fyi for this PR to be merged you need to also update the specs and update all other occurrences of ::Device and ::App - specs are currently failing in travis because of that

colinta commented 10 years ago

Not the same, of course, but it's definitely not considered good practice to add a class to the global namespace without a prefix, that's Obj-C 101... we get away with it because it's Ruby. :smiley:

rantrix commented 10 years ago

@clayallsopp I've made the update to the spec files. All is well on Travis CI build. Thank you.