-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Fix implicitNotFound message for type aliases #19343
Conversation
c550fdc
to
06f086b
Compare
if (updateCheckFiles) { | ||
FileDiff.dump(checkFile.toPath.toString, actual) | ||
echo("Updated checkfile: " + checkFile.getPath) | ||
} else { | ||
onFailure(testSource, reporters, logger, Some(msg)) |
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.
Originally I left this one out to force users to re-run the tests against the new checkfile. This can be useful if there is a source of non-determinism in the output.
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 can revert this. Was pretty confusing initially but I might be biased by the Scala 2 experience.
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.
You can keep your change. There is no right or wrong way to do this. Your way might be easier to understand.
Any chance to get a review? I'm not sure who to ping. I guess the 3.2 ship has sailed, but I hope we can backport it to 3.3 |
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.
Just two minor test/doc comments, otherwise looks good to me.
@@ -2784,19 +2784,25 @@ class MissingImplicitArgument( | |||
val idx = paramNames.indexOf(name) | |||
if (idx >= 0) Some(i"${args(idx)}") else None | |||
"""\$\{\s*([^}\s]+)\s*\}""".r.replaceAllIn(raw, (_: Regex.Match) match | |||
case Regex.Groups(v) => quoteReplacement(translate(v).getOrElse("")).nn | |||
case Regex.Groups(v) => quoteReplacement(translate(v).getOrElse("?" + v)).nn | |||
) | |||
|
|||
/** @param rawMsg Message template with variables, e.g. "Variable A is ${A}" | |||
* @param sym Symbol of the annotated type or of the method whose parameter was annotated | |||
* @param substituteType Function substituting specific types for abstract types associated with variables, e.g A -> Int |
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 documentation should be updated to include the new parameters.
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.
Done 👍
@@ -2784,19 +2784,25 @@ class MissingImplicitArgument( | |||
val idx = paramNames.indexOf(name) | |||
if (idx >= 0) Some(i"${args(idx)}") else None | |||
"""\$\{\s*([^}\s]+)\s*\}""".r.replaceAllIn(raw, (_: Regex.Match) match | |||
case Regex.Groups(v) => quoteReplacement(translate(v).getOrElse("")).nn | |||
case Regex.Groups(v) => quoteReplacement(translate(v).getOrElse("?" + v)).nn |
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.
We should maybe have a separate test case for the "?" case?
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.
We have this test case:
26 | summon[H[Int]] // error
| ^
| Not found for Int, ?B
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.
Yes, I wrote a separate test case because I think it could deserve its own tests, but that's okay like that.
06f086b
to
d34ab6d
Compare
Fix #7092