ScannerImpl.getToken() and ScannerImpl.peekToken() not correctly implemented?

[Sylvain Baudoin]

Hello,

The methods getToken() and peekToken() are not correctly implemented:

• getToken() never calls needMoreTokens(), which causes the method to return null if called a second time withtout peekToken() in between.
• peekToken() may raise an IndexOutOfBoundsException instead of returning null as it does not check if there are tokens left before calling tokens.get(0).

These issues causes the scanner not to work as expected, especially compared to its Pythonic brother.

[Andrey Somov]

Do you mean: The methods getToken() and peekToken() are not correctly DOCUmented ? The profiler reports that these 2 methods are called a lot. The extra calls are not required by SnakeYAML itself and they would cause nothing but a performance degradation.

Of course, they should be be properly used outside of SnakeYAML. But this is a matter of proper documentation.

[Andrey Somov]

1. getToken() gets a better Javadoc
2. peekToken() is implemented and documented properly. But the PyYAML implementation is redundant. I have created a fix. Feel free to add your comments.

[Sylvain Baudoin]

No, it's not a matter of documentation. On the contrary, the doc says that the difference between peekToken() and getToken() relies only on fact that in a method the token remains in the stack, not in the other.

IMHO, I'd stick with the implementation of PyYAML as follows:

    /**
     * Return the next token, but do not delete it from the queue.
     */
    public Token peekToken() {
        while (needMoreTokens()) {
            fetchMoreTokens();
        }
        if (!this.tokens.isEmpty()) {
            return this.tokens.get(0);
        }
        return null;
    }

    /**
     * Return the next token, removing it from the queue.
     */
    public Token getToken() {
        while (needMoreTokens()) {
            fetchMoreTokens();
        }
        if (!this.tokens.isEmpty()) {
            this.tokensTaken++;
            return this.tokens.remove(0);
        }
        return null;
    }

In all cases the IndexOutOfBoundsException that peekToken() may raise is really a problem that must be fixed. Becaus it is not mentioned in the doc (unless you did it) but above all because this is a major behavioral difference between peekToken() and getToken().

[Andrey Somov]

Please read what I have already said before.

1. The Javadoc has been already updated
2. The while loop in getToken() was removed because it is not required
3. The if statement in peekToken() is not required. I have created a patch for PyYAML

[Sylvain Baudoin]

OK with the doc, it is clearer, but this make the method getToken() to work differently from the reference implementation of PyYAML.

I can't say about the while loop.

About the patch for PyYAML, I cannot speak on behalf of the people responsible for the project but I disagree with it because it changes the way peekToken() and getToken() currently works: the if makes that the function will return None if self.tokens is empty. With your patch we will get an IndexError, making the function to behave like snakeyaml that currently raises an IndexOutOfBounsException. I'd prefer snakeyaml to implement the if and return null instead. But yes, if correctly documented we can handle this exception.

[Andrey Somov]

The patch for PyYAML has a value:

1. Remove the code which does not do anything
2. Make the code work quicker

If someone is going to use the low level API (does not matther PyYAML or SnakeYAML) he has to call the methods in the proper order.

I do not see the reason to introduce redundant code in SnakeYAML instead of removing redundant code in PyYAML.

[Andrey Somov]

PyYAML has been changed to work the same way as SnakeYAML (https://github.com/yaml/pyyaml/pull/150)

Thank you for the help.

[Sylvain Baudoin]

I still have questions, though. I looked at your doc update but I don't see how to use checkToken() is a generic way. I'm working on a project for which I need a generic scanner without knowledge of the next token to come: I just want to get the tokens as they come. How to do that with checkToken()?

[Andrey Somov]

You do not have to know the next token to come. Just call checkToken()

(by the way, it works the same way in PyYAML)

[Sylvain Baudoin]

Indeed, I did not pay enough attention to the code of checkToken(). Thanks.

By the way, again, I would mention in the Javadoc of Scanner.peekToken() that a IndexOutOfBoundsException will be raised if there is no more token to be returned (and so use checkToken() beforehand).

[Sylvain Baudoin]

Sorry to bother again but why not also remove the while loop in peekToken()? From a functional point of view, as the only difference between peekToken() and getToken() is a "stack-like operation" there is no justification for this while loop in peekToken() as well. I don't really understood why we should call checkToken() before getToken() and not before peekToken(). If we decide to externalize the check because it is already done in checkToken() it must be externalized for both peek and get methods. WDYT?

[Andrey Somov]

Feel free to refactor and create a pull request.

[Andrey Somov]

This is implemented in the release 1.21: Scanner throws IndexOutOfBoundsException if no more token left but getToken() or peekToken()

[Andrey Somov]

Well, it looks like it is a real issue.

[Sylvain Baudoin]

!!! How did you come to that conclusion? I managed to have my code compatible with both version (1.20 and 1.21) by the way.

[Andrey Somov]

It would be better if tokens and events have the same behaviour.

[Andrey Somov]

It will addressed in the re-build of SnakeYAML