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

Remove instantiating Device as BW::Device #355

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Remove instantiating Device as BW::Device #355

wants to merge 1 commit into from

Conversation

rantrix
Copy link

@rantrix rantrix commented Mar 26, 2014

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
Copy link
Contributor

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

  • Do you feel similarly about BubbleWrap's App shorthand?
  • Since it is backwards incompatible, would be a BW 2.X level change and wouldn't be merged soon

@markrickert
Copy link
Collaborator

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
Copy link
Contributor

colinta commented Mar 27, 2014

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
Copy link
Author

rantrix commented Mar 27, 2014

@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
Copy link
Contributor

@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
Copy link
Contributor

colinta commented Mar 28, 2014

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. 😃

This will prevent namespace conflicts.
@rantrix
Copy link
Author

rantrix commented Mar 31, 2014

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

@clayallsopp clayallsopp added the 2.0 label May 7, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants