-
Notifications
You must be signed in to change notification settings - Fork 279
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
Don't use deprecated scalameta methods #3426
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.
Could we not use .internal.Impl
especially when it can be avoided.
case ParamClauseParent( | ||
t @ Defn.ExtensionGroup.internal.Impl(_, b: Term.Block) | ||
) if isBlockStart(b, nft) => |
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.
case ParamClauseParent( | |
t @ Defn.ExtensionGroup.internal.Impl(_, b: Term.Block) | |
) if isBlockStart(b, nft) => | |
case ParamClauseParent( | |
t : Defn.ExtensionGroup | |
) if isBlockStart(t.body, nft) => |
I would avoid using the internal Impl classes since they might not be binary compatible.
Some(new OptionalBracesRegion { | ||
def owner = Some(t) | ||
def splits = Some(getSplitsMaybeBlock(ft, nft, b)) | ||
def rightBrace = blockLast(b) | ||
}) | ||
case t @ Term.If(_, thenp, _) if !nft.right.is[T.KwThen] && { | ||
case t @ Term.If.internal.Impl(_, thenp, _, _) |
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.
case t @ Term.If.internal.Impl(_, thenp, _, _) | |
case t : Term.If |
and then just use t.thenp
Some(new OptionalBracesRegion { | ||
def owner = Some(t) | ||
def splits = Some(getSplitsMaybeBlock(ft, nft, b)) | ||
def rightBrace = blockLast(b) | ||
}) | ||
case t @ Term.If(_, thenp, _) if !nft.right.is[T.KwThen] && { | ||
case t @ Term.If.internal.Impl(_, thenp, _, _) |
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.
case t @ Term.If.internal.Impl(_, thenp, _, _) | |
case t : Term.If |
and then just use t.thenp.
Also in other places in the file
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.
will do. however, why would we need (or expect) binary compatibility here? for every upgrade of the parser, we explicitly modify the formatter. the whole point to add those internal.Impl.unapply
methods was for a case like this.
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.
Hmm... binary compat might not be a problem yeah. Maybe I went overeager, but I still think it's a bit easier to just annotate type
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
No description provided.