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

Minor cleanup #21

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

Minor cleanup #21

wants to merge 3 commits into from

Conversation

rfestag
Copy link

@rfestag rfestag commented Apr 21, 2017

Removed unnecessary files:
.rspec provided options no longer supported.
redis_ext is no longer necessary. The redis gem properly identifies the driver from the symbol provided.

Updated tests to use :expect instead of :should semantics (which were deprecated in RSpec)

I actually started this because I was having an unrelated issue (I needed to use ::Redis insetad of Redis when instantiating a client in a Celluloid actor). If you are interested, I might refactor a bit so there is no Celluloid::Redis module to conflict with Redis within an actor. I always found prefixing classes with :: to force kernel scope to be a little unclean, but I understand if you'd rather keep things as they are.

@tarcieri
Copy link
Member

This isn't a "minor cleanup", it's a breaking API change.

I'd suggest this instead:

class Celluloid::Redis < ::Redis

… collision with Redis within actors"

This reverts commit 285f1ac.
@rfestag
Copy link
Author

rfestag commented Apr 23, 2017

Sorry for the confusion, that wasn't originally intended to be part of this. I don't often do pull requests, and forgot that pushing changes after the pull would be included. The "minor cleanup" was to remove monkey patching that isn't necessary anymore, and to clean up RSpec tests and the .rspec file. I can issue a separate pull request with the change described above.

@rfestag
Copy link
Author

rfestag commented Apr 23, 2017

I looked a little closer, and I think the snippet above would constitute a breaking change as well, since Celluloid::Redis is currently a module, not a class. That said, I did test it, and it does work. If I do commit it, perhaps it would be time to bump the version to something that indicates a breaking API change. I think this is cleaner than what I did originally.

--color
--format documentation
--backtrace
--default_path spec
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you delete this?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because rspec wouldn't run with any of those options. I assume they were removed.

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.

2 participants