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

Change the return type of TokenList's prev/next to Option[Token] #517

Closed
marcelocenerine opened this issue Dec 19, 2017 · 4 comments
Closed

Comments

@marcelocenerine
Copy link
Contributor

The prev/next methods in TokenList return the input token if it is the head or last element of the tokenList:

val tokens = "".tokenize.get // this contains two tokens: BOF and EOF
val tokenList = TokenList(tokens) 

assert(tokenList.prev(tokens.head) == tokens.head)
assert(tokenList.next(tokens.last) == tokens.last)

In order to identify when a tokenList has run out of tokens while traversing it using prev or next, users must check whether the returned value is the same object passed as input, which is not a pretty standard thing in Scala libraries and can cause confusion. Maybe it would be clearer if those methods returned Option[Token] instead:

def next(token: Token): Option[Token]
def prev(token: Token): Option[Token]
@marcelocenerine
Copy link
Contributor Author

I have this one implemented already but will wait until #534 (hopefully) gets merged.

@olafurpg
Copy link
Contributor

olafurpg commented Jan 3, 2018

@marcelocenerine I am afraid changing the signature is a breaking change that I'd like to avoid. I personally also like the current semantics since I use it regularly to do quick guards
like

+            case lf @ Token.LF() if ctx.tokenList.prev(lf).is[Token.Comma] =>

in #537 What do you think about adding instead nextOption/prevOption ?

@marcelocenerine
Copy link
Contributor Author

marcelocenerine commented Jan 4, 2018

@olafurpg that's true :(. If you want to avoid that and like the current semantics, then let's keep them as is.
If the api provided both next and nextOption, I think I would expect that next would throw if there is no next token (same for prev). Maybe we just add some basic scaladoc for now? Feel free to close this issue.

@olafurpg
Copy link
Contributor

olafurpg commented Jan 4, 2018

If the api provided both next and nextOption, I think I would expect that next would throw if there is no next token (same for prev).

I agree, I'm hesitant to add nextOption for this reason too.

I opened #539 to add a docstring to next/prev. Please take a look to see if the docstring can be clearer.

@olafurpg olafurpg closed this as completed Jan 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants