-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Enchancement #3008 #3228
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
Enchancement #3008 #3228
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! ☀️
Hi! This already looks pretty good, a few improvements that could be made:
|
By the way, to check that the modified test works correctly, you can run just the REPL printing tests by typing in sbt: dotty-compiler/test-only -- --tests=typePrinterTests |
@@ -126,8 +126,8 @@ class RefinedPrinter(_ctx: Context) extends PlainPrinter(_ctx) { | |||
def isInfixType(tp: Type): Boolean = tp match { | |||
case AppliedType(tycon, args) => | |||
args.length == 2 && | |||
!Character.isUnicodeIdentifierStart(tycon.typeSymbol.name.toString.head) | |||
// TODO: Once we use the 2.12 stdlib, also check the @showAsInfix annotation | |||
!Character.isUnicodeIdentifierStart(tycon.typeSymbol.name.toString.head) && |
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.
This should be an ||
not an &&
as we want infix types to be used either for type constructors which start with a symbol or that have the @showAsInfix
annotation.
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.
There's one more problem. I didnt notice that this annotation takes a boolean argument, which should be checked as well.
@smarter Thanks for pointing me to tests, looks like there's a problem - |
Ah you're running into a known bug (#2462), you can work around it by using the fully qualified name of the annotation e.g: scala> @scala.annotation.showAsInfix type Mappy[T, U] = Map[T, U] |
d5b83d8
to
1a2ad9c
Compare
Refined printer checks if shoAsInflixAnotType annotation is used. Based on "SI-4700 Make infix notation default for symbolic types" from original scala repository
1a2ad9c
to
5d70d1d
Compare
@smarter I added tests, the only problem I have right now is using type alias |
Hmm this is probably the Dotty typechecker being too eager to dealias things, I'll look into it but it might be tricky to fix. We can merge this PR without the type aliases working since it already improves things anyway. |
@smarter so my proposition is to merge this pull request and open a new issue where someone (maybe even me, with some guidance :) ) can check how to approach type aliases |
LGTM, thanks for your contribution! |
I'm afraid this might be a hard issue to fix :) I suggest you take a look at https://github.com/lampepfl/dotty/issues?q=is%3Aissue+is%3Aopen+label%3Aexp%3Anovice or ask on http://gitter.im/lampepfl/dotty if you're looking for other things to contribute. |
Thanks for your guidance and help :) |
Refined printer checks if showAsInflisAnotType annotation is used (issue #3008).
I would be grateful if you could lend me a hand in testing my solution, couldn't find any tests for RefinedPrinter.
If something is missing please let me know, i'm happy to improve this PR :)