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

Namespacing "Analytics" Constant #15

Closed
nickL opened this issue Aug 1, 2013 · 6 comments
Closed

Namespacing "Analytics" Constant #15

nickL opened this issue Aug 1, 2013 · 6 comments

Comments

@nickL
Copy link

nickL commented Aug 1, 2013

Would there by any consideration of namespacing the top-level "Analytics" constant or perhaps the AnalyticsRuby module?

I've found issues of it clashing with internal classes named "Analytics" since its defined as top-level namespace (https://github.com/segmentio/analytics-ruby/blob/master/lib/analytics-ruby.rb#L35)

I can work on a PR and submit it if you're interested, however just wanted to throw out the idea as an improvement. "Analytics" is such a general name so I imagine it would cause some conflicts down the road..

In any case, great work guys. Looking forward to the service growing. 👍

@calvinfo
Copy link
Contributor

calvinfo commented Aug 1, 2013

This is probably not something we'll do anytime soon, but I hear your concerns :(.

You're definitely right from a developer perspective. We should be namespaced for a namespace that we own. We're mostly trying to get uniformity across our different libraries and make installing easy for the average case. If we do get a bunch of complaints though, we'd consider dropping the top-level Analytics naming.

In the meantime, the best thing to do is fork and remove that line (which it looks like you already have). I remember a pull request wanting to do something similar by creating their own module instead: #9

@calvinfo calvinfo closed this as completed Aug 1, 2013
@coryschires
Copy link

+1 for adding a namespace.

It's definitely common, best practice to namespace ruby gems, especially if they use common names like Anayltics.

Taking a look at this gem, I'd suggest completely removing AnaylticsRuby (since that wrongly suggests you're profiling Ruby code) and namespace everything under SegmentIo. Then, for example, you'd have:

SegmentIo::Anayltics
SegmentIo::Client
SegmentIo::Response
# And so on ...

The public API then would be accessed under that namespace as well. For example:

SegmentIo::Anayltics.identify ...

If you're concerned with consistency, you could apply this pattern to your other libraries as well. It seems like the rationale for namespacing would apply equally to Python, PHP, etc. It would be a move in the right direction.

As far a backwards compatibility, it would a simple update for folks currently using the old API: just add the SegmentIo namespace. Or, if that's too much trouble, create an initializer with something like:

Analytics = SegmentIo::Anayltics

I totally understand this is probably not a priority. But it's obviously wrong, and I would be willing to do all the work and make a pull request.

Any chance you'd reconsider?

@jakswa
Copy link

jakswa commented Oct 1, 2013

+1 because I just tried to add your gem to a rails app named "Analytics". I typed up more info in the dupe issue linked above. 😒

@rustybailey
Copy link

+1

calvinfo added a commit that referenced this issue Oct 4, 2013
Fixes #15, and allows the developer to choose the namespace. Our
library will only use the namespace we _actually_ own, and we've
updated the docs to alias to Analytics manually if the user so
desires.
@calvinfo
Copy link
Contributor

calvinfo commented Oct 4, 2013

hey guys, yeah, this has bothered me for a while. just got around to fixing it and updating our docs momentarily.

thanks for the patience!

@olivierlacan
Copy link

Fun story. We had defined a Segment module under which a Base class and a few other subclasses were defined. When you folks changed the namespace to Segment::Analytics, all the calls to Analytics.track inside our Segment::Base were inferred as Segment::Analytics.track calls which obviously didn't work.

I spent way too much time scratching my head on this one. So while I was fully aware of the namespace change, I was still bit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants