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

Allow with-chain wrapping for all params #1125 #1169

Merged
merged 1 commit into from May 9, 2018

Conversation

iantabolt
Copy link
Contributor

Back to #1125, this covers parameter types for with-chains.

Currently,

class NeedsServices(
    services: Bag
      with SomeOtherService.RequiredServices
      with HasDynamicConfig
      with HasThrottles
      with HasThrowableNotifier
)

ends up as

class NeedsServices(
    services: Bag with SomeOtherService.RequiredServices with HasDynamicConfig with HasThrottles with HasThrowableNotifier
)

With this change it will wrap it as expected with one with per line.

Copy link
Member

@olafurpg olafurpg left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Thanks!

// val/def/var/type definitions or declarations
case (top: Type.With) `:parent:` (_: Defn | _: Decl) =>
// self types, params, val/def/var/type definitions or declarations
case (top: Type.With) `:parent:` (_: Defn | _: Decl | _: Term.Param) =>
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we hit a regression by simply removing this guard 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Just tried and it didn't come out great, need tree printer to properly fix this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I tried removing the parent guard when playing around with my original PR and it ended up wrapping after the second with sometimes. I was thinking you need to at least make sure that parent is not another Type.With but TreeOps.topTypeWith should guarantee that 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Life is a lot easier in https://github.com/scalacenter/scala-syntax where we print code by traversing the tree instead of visiting the parent chain up from tokens as we do here.

@olafurpg olafurpg merged commit 0bb92a2 into scalameta:master May 9, 2018
@olafurpg olafurpg added this to the v1.6.0 milestone May 27, 2018
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

2 participants