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

[feature] - Add rejson bindings #41

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

Conversation

codeslayer1
Copy link

Feature:

  • Add rejson bindings for redis in case ENABLE_REJSON flag is set in process.env

Use Case:

  • Can be used to enable rejson commands with redis client for those using rejson module

@codeslayer1
Copy link
Author

@nelsonic Hey. Can you please check this pull request and let me know if it can be merged since I need to use ReJSON with my project.

The tests seem to be failing for some reason even without any changes. I have added a test for my changes as well.

@nelsonic
Copy link
Member

@codeslayer1 thanks for opening this feature/pull request. 👍
Indeed the tests failing is a "blocker" to us merging this PR.
https://travis-ci.org/dwyl/redis-connection/builds/461732661
image

is rather cryptic in how it's failing ...
I will investigate when I get back to my desk.
Meanwhile are you able to use a local version in your project to tide you over?

@codeslayer1
Copy link
Author

Hey. Thanks for your quick response.

Yes. I was able to use ReJSON with my local build. Since I needed this feature urgently, I have published a forked package on npm with rejson (redis-connection-manager). Will revert back to this package once the pull request is merged and an updated version is published.

Please let me know in case any help/update is needed at my end to make the tests pass and get this PR merged.

[![devDependency Status](https://david-dm.org/dwyl/redis-connection/dev-status.svg)](https://david-dm.org/dwyl/redis-connection#info=devDependencies)
<!-- [![HitCount](https://hitt.herokuapp.com/nelsonic/redis-connection.svg)](https://github.com/dwyl/redis-connection) -->
[![npm](https://img.shields.io/npm/v/redis-connection.svg)](https://www.npmjs.com/package/redis-connection)
[![npm](https://img.shields.io/npm/v/redis-connection-manager.svg)](https://www.npmjs.com/package/redis-connection-manager)
Copy link
Member

Choose a reason for hiding this comment

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

@codeslayer1 you've pushed changes on this PR which pertain to your forked package. Please revert these before fixing failing tests. 👍

@codeslayer1
Copy link
Author

Hey. I have raised a fresh pull request with the Readme and package.json changes that pertain to my package reverted. Please use that new pull request to merge the rejson change. You can delete this pull request.

@nelsonic nelsonic removed their assignment Dec 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants