-
Notifications
You must be signed in to change notification settings - Fork 185
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
Issue 319 #444
Issue 319 #444
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this contribution, and catching the bug!
|
||
test("Prev returns the firstToken when is given the firstToken") { | ||
val tokens = "".tokenize.get // this contains two tokens: beginningOfFile and endOfFile | ||
val firstToken = tokens.last |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tokens.head
?
@@ -53,7 +53,7 @@ class TokenList(tokens: Tokens) { | |||
|
|||
def prev(token: Token): Token = { | |||
tok2idx.get(token) match { | |||
case Some(i) if tokens.length > i - 1 => | |||
case Some(i) if i - 1 > 0 => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i > 0
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tokens(0)
should be possible
@@ -53,7 +53,7 @@ class TokenList(tokens: Tokens) { | |||
|
|||
def prev(token: Token): Token = { | |||
tok2idx.get(token) match { | |||
case Some(i) if tokens.length > i - 1 => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch! Bad copy paste 😅
import scala.meta._ | ||
import scalafix.util.TokenList | ||
|
||
class TokenListTest extends FunSuite { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great idea 👍
} | ||
|
||
test("Slice gives an empty list with same token as inputs") { | ||
val tokens = "val x = 2".tokenize.get |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need val x = 2
? It seems like we could reuse the same "".tokenize.get
by putting in the top of the suite before the first test
. That would to write small and declarative tests without duplicating new TokenList(..)
in every unit test.
Sorry, I rushed a bit XD |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍 Nice work!
Closes #319
Thanks to @GoldenZero