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

buigfix: given scala toplevel mtags #5247

Merged
merged 2 commits into from May 22, 2023

Conversation

kasiaMarek
Copy link
Contributor

@kasiaMarek kasiaMarek commented May 19, 2023

Previously:
In ScalatopLevelMtags we expected an identifier after given keyword, so we would fail on given [A]....
Now:

  1. We check if it's an identifier and emit a term only if after the identifier there is a colon with only args and params in between.
  2. We don't throw exceptions in ScalatopLevelMtags on an error, instead we create a report and keep indexing.

@kasiaMarek kasiaMarek marked this pull request as ready for review May 19, 2023 13:36
Copy link
Member

@jkciesluk jkciesluk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! One nitpick, we should allow whitespace before COLON

|given [T]: List[T] = Nil
|given given_Char: Char = '?'
|given `given_Float`: Float = 3.0
|given `* *` : Long = 5
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
|given `* *` : Long = 5
|given `* *`: Long = 5

I was confused why * * didn't show in result, but it was because of a whitespace.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, you're so right. I forgot that also turns into a token. Thanks for noticing.

consumeParams()
scanner.curr.token match {
case COLON => identifier
case _ => None
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will fail if there is a whitespace before COLON

Copy link
Contributor

@tgodzik tgodzik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@kasiaMarek kasiaMarek requested a review from jkciesluk May 22, 2023 09:45
Copy link
Member

@jkciesluk jkciesluk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

@kasiaMarek kasiaMarek merged commit 217bf61 into scalameta:main May 22, 2023
22 of 23 checks passed
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

Successfully merging this pull request may close these issues.

None yet

3 participants