Skip to content
This repository has been archived by the owner on Dec 7, 2018. It is now read-only.

A bit of refactoring and address issue #17 #18

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Conversation

niamster
Copy link

No description provided.

@niamster
Copy link
Author

@tarcieri I'm not sure if this is what you've expected but I have no better idea how to avoid Celluloid::Redis::VERSION

@tarcieri
Copy link
Member

So, maybe I did this whole gem wrong originally.

Perhaps it shouldn't use the Celluloid namespace at all (which is already quite polluted)

Maybe the gem should be called redis-celluloid-io and place things into the Redis::Celluloid::IO (or Redis::CelluloidIO namespace, and call the gem redis-celluloidio)

I think the way things are structured now makes them weirdly coupled.

@digitalextremist
Copy link
Member

I'm about to cut a pre-alpha of Reel::IO and it seems like Redis::IO would even work.

@tarcieri
Copy link
Member

Given Redis isn't celluloid-specific, I feel like the module/class name should call out Celluloid in some way

@niamster
Copy link
Author

I tend to agree with @tarcieri but on the other hand it would be pain for the users to update the name of the gem, etc.
@digitalextremist I'm not sure Redis::IO is a good name as it might badly collide with future redis-rb development.

I would probably name this redis-connector-celluloid which makes clean definition on what the gem does. It would also permit clear definition of VERSION constant.

@digitalextremist
Copy link
Member

Hahah... Celluloid::Red

I think Redis::Celluloid sucks the least.

@tarcieri
Copy link
Member

@niamster I wouldn't worry about backwards compatibility. The external API of the gem is tiny and serves only to enable Celluloid as a redis-rb driver plugin.

@niamster
Copy link
Author

So here we have a couple of names:

  • redis-io
  • redis-celluloid-io
  • redis-celluloid
  • redis-connector-io
  • redis-connector-celluloid
  • redis-connector-celluloid-io

IMHO redis-celluloid-* and redis-io do not solve original issue nor it's obvious what the gem does. The last two names are quite long.
My vote is for redis-connector-io or redis-ccio as a short for the last name.

@tarcieri
Copy link
Member

The whole point is it adds a Celluloid::IO driver so the gem should mention that in some way.

Perhaps there's an existing convention? Are there any other redis-rb drivers packaged as gems?

How about:

  • redis-celluloid-driver
  • redis-celluloidio-driver

?

@marshall-lee
Copy link

Why connector?
Simply redis-connection-celluloid — just how the class named.

@tarcieri
Copy link
Member

@marshall-lee +1 to the Gem name reflecting the module/class names

@marshall-lee
Copy link

...and maybe also put a VERSION constant into Redis::Connection::Celluloid? only one file remains :)

@tarcieri
Copy link
Member

the VERSION constant should be placed under GEM_NAME.underscore::VERSION IMO

@marshall-lee
Copy link

@tarcieri ah, yeah. i've seen this before in other gems, it makes sense.

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

Successfully merging this pull request may close these issues.

4 participants