Something to generalize accepted types #345

Closed
wants to merge 1 commit into
from

Projects

None yet

4 participants

@codemercenary

I know that redis only accepts string types, but technically there's no reason not to accept those same types when passed as arguments. I've created a new method called "isPrimitive" which detects whether the passed value is a string, integer, or float, and allows any of these to be coerced to a string type.

Contributor
DTrejo commented Feb 18, 2013

This change, where strings are required, was a mistake on my part that broke backwards compatibility.

Similar issue: #335.

My plan is to undo it, and only mention this in the docs. does that sound reasonable?

Owner

@DTrejo was there an issue that caused you to make the original change? I agree with the undo, though would want to make sure we aren't regressing.

@MetaThis MetaThis commented on the diff Feb 21, 2013
@@ -945,8 +945,8 @@ RedisClient.prototype.hmset = function (args, callback) {
for (i = 0, il = tmp_keys.length; i < il ; i++) {
key = tmp_keys[i];
tmp_args.push(key);
- if (typeof args[1][key] !== "string") {
- var err = new Error("hmset expected value to be a string", key, ":", args[1][key]);
+ if (isPrimitive(args[1][key])) {
MetaThis
MetaThis Feb 21, 2013

Is this meant to be !isPrimitive(...) rather than isPrimitive(...)?

@DTrejo DTrejo added a commit that referenced this pull request Feb 24, 2013
@DTrejo DTrejo Revert "hmset throws/errors out on non-string values. fixes #218"
Reverting because this was a documentation problem, not a problem with
the code. Performance-wise, this is faster than the approach in #345, though
it may cause users more trouble. This is okay, if someone opens an issue we
can point them to the docs.

This reverts commit b60e001.

Conflicts:

	index.js
	test.js
405011b
Contributor
DTrejo commented Feb 24, 2013

This should no longer give people any trouble.

Thanks for speaking up,
David

@DTrejo DTrejo closed this Feb 24, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment