SCAN cursor is int, should be unsigned long

[playerofgames]

Great that SCAN etc. are in latest release. The official documentation describes the cursor as a "string representing an unsigned 64 bit number", whereas the jedis methods use ints for cursors. Obviously, long type doesn't quite get us there either unless we mess with the sign, but would presumably be closer to compatibility with documented spec. I'd vote for strings personally.

[xetorthio]

Yes. This makes lots of sense. This will break backward compatibility, so we should be careful on when we should a merge a fix for this.

[HeartSaVioR]

I agree to be careful on breaking backward compatibility. :)

But I also think we should think more about this issue, because it's not a simple functional lack, it's a bug.

Let me explain this issue by showing current implementation to scan.
(It's same to all *scan commands).

public ScanResult<String> scan(int cursor, final ScanParams params) {
    checkIsInMulti();
    client.scan(cursor, params);
    List<Object> result = client.getObjectMultiBulkReply();
    int newcursor = Integer.parseInt(new String((byte[]) result.get(0)));
    List<String> results = new ArrayList<String>();
    List<byte[]> rawResults = (List<byte[]>) result.get(1);
    for (byte[] bs : rawResults) {
        results.add(SafeEncoder.encode(bs));
    }
    return new ScanResult<String>(newcursor, results);
    }

*scan methods may throw NumberFormatException when Redis returns cursor value upper than signed integer's max, and there's no try/catch statement in Jedis.
So user may receive NumberFormatException directly, and user could be confused because it's not user request problem at all.

Modifying this will break compatibility only related to *scan commands, so only users who uses *scan command feel inconvenience. (Am I right?)
Then we could think about feel more inconvenient by "bug" vs "compatibility".

I wish to listen our opinions about it. :)

[marcosnils]

@HeartSaVioR I do agree that it is a bug. Since SCAN was introduced in the last release, maybe we can merge your PR now and release a minor with the change.

What do you think?

[xetorthio]

Following semver, patches need to be backward compatible bugfixes. And this
isn't backward compatible.
I am very tempted to add this asap, but we should try to respect semver.
So I propose to do the following:

1. Leave all current SCAN commands but add to them @decrecated
   annotation.
2. Add new commands that receives a String cursor.
3. Add to ScanResult class a getStringCursor and add
   @decrecated to the current getCursor method. Also explain that
   on the next major release we'll remove scan commands with integer cursor,
   and that we'll remove the integer version of getCursor and rename
   getStringCursor to getCursor.
4. Create a Pull Request that actually removes the integer version of the
   commands and renames getStringCursor to getCursor and schedule
   it for the "Next major" milestone.

By doing this, we are backward compatible and we can release this in the
next minor release.

We should also learn from this (specially me :) ) to pay more attention
when implementing new commands :)

On Feb 9, 2014 2:06 PM, "Marcos Nils" notifications@github.com wrote:

@HeartSaVioR https://github.com/HeartSaVioR I do agree that it is a
bug. Since SCAN was introduced in the last release, maybe we can merge your
PR now and release a minor with the change.

What do you think?

Reply to this email directly or view it on GitHubhttps://github.com//issues/531#issuecomment-34582786
.

[HeartSaVioR]

@marcosnils @xetorthio I respect your thought. :)

Though I really wanna follow @marcosnils and let this PR to next minor, I also agree to try to respect policies we have, so I'm following @xetorthio .
It would a good practice to make and respect some policies between us. :)

So what would be nice to name new *scan method?
I'm ready to make PRs proposed by you. :)

[marcosnils]

What about overloading the current *scan methods but instead of using an int jursor just send a string?If we annotate the methods with a good description it shouldn't be a problem

[xetorthio]

Exactly. Overloading :)
On Feb 9, 2014 4:00 PM, "Marcos Nils" notifications@github.com wrote:

What about overloading the current *scan methods but instead of using an
int jursor just send a string?If we annotate the methods with a good
description it shouldn't be a problem

Reply to this email directly or view it on GitHubhttps://github.com//issues/531#issuecomment-34586388
.

[HeartSaVioR]

@xetorthio @marcosnils I've just found that it's overloadable! Thank you all! :)

[HeartSaVioR]

@marcosnils @xetorthio I open new pull request cleaned one, #536.
Let's review on this! :)

[marcosnils]

Perfect, I'm closing this one so we don't get confused!.