-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add error message for wrong number of argument patterns #3508
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
Add error message for wrong number of argument patterns #3508
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.
Hello, and thank you for opening this PR! 🎉
All contributors have signed the CLA, thank you! ❤️
Have an awesome day! ☀️
assertMessageCount(1, messages) | ||
val UnapplyInvalidNumberOfArguments(qual, argTypes) :: Nil = messages | ||
assertEquals("Boo", qual.show) | ||
assertEquals("(class Int, class String)", s"(${argTypes.map(_.typeSymbol).mkString(", ")})") |
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.
Use argTypes.map(_.typeSymbol).mkString("(", ", ", ")")
instead of s"(${argTypes.map(_.typeSymbol).mkString(", ")})"
.
|Expected usage would be something like: | ||
|case $qual(${argTypes.map(_ => '_')}%, %) => ... | ||
| | ||
|where subsequent arguments would have following types: ($argTypes%, %).""".stripMargin |
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.
""".stripMargin
should be on the next line.
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.
The issue with stripMargin
being on the next line is that it adds an extra line to the explanation which is not desired.
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.
@nicolasstucki @allanrenucci can you please decide which option is recommended? I will gladly oblige.
Also - whoever does the merge can fix it as I have checked 'Allow edits from maintainers'.
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.
Use Allan's.
ce1313e
to
e5e9270
Compare
@nicolasstucki thanks for such a quick review! I have reflected your comments. |
Many thanks! |
Refs #1589