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

Fix early initializers #3091

Closed
wants to merge 1 commit into from
Closed

Fix early initializers #3091

wants to merge 1 commit into from

Conversation

dos65
Copy link
Member

@dos65 dos65 commented Jan 31, 2022

No description provided.

Comment on lines +560 to +565
object CoreReflection extends {
val u: ru.type = ru
val mirror: u.Mirror =
u.runtimeMirror(classOf[scala.meta.Tree].getClassLoader)
} with scala.meta.internal.trees.Reflection
with scala.meta.internal.tokens.Reflection
Copy link
Member Author

Choose a reason for hiding this comment

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

Without this fix it's formatted as:

  object CoreReflection
      extends {
        val u: ru.type = ru
        val mirror: u.Mirror = u.runtimeMirror(classOf[scala.meta.Tree].getClassLoader)
      }
      with scala.meta.internal.trees.Reflection
      with scala.meta.internal.tokens.Reflection

Copy link
Collaborator

Choose a reason for hiding this comment

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

actually, i would think this new formatting is NOT desirable. why is extends treated differently than with?

how is this formatted:

object CoreReflection
      extends Foo(
        bar,
        baz
      )
      with scala.meta.internal.trees.Reflection
      with scala.meta.internal.tokens.Reflection

Copy link
Member Author

Choose a reason for hiding this comment

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

From what I see, currently the logic is designed for the case of extends-with chain and then template body.
The early-init is a different case - there is a body between extends and with chain.

I think it would be better to support this construction if the required amount of changes is similar to what I proposed. That's the style how they were used + Scalafmt 2 worked worked well with it + migration to 3 triggers unwanted reformat.

Copy link
Collaborator

Choose a reason for hiding this comment

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

see #3094, it does something similar but consistent with configuration. the difference is indentation.

Copy link
Collaborator

@kitbellew kitbellew left a comment

Choose a reason for hiding this comment

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

-1

Comment on lines +560 to +565
object CoreReflection extends {
val u: ru.type = ru
val mirror: u.Mirror =
u.runtimeMirror(classOf[scala.meta.Tree].getClassLoader)
} with scala.meta.internal.trees.Reflection
with scala.meta.internal.tokens.Reflection
Copy link
Collaborator

Choose a reason for hiding this comment

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

actually, i would think this new formatting is NOT desirable. why is extends treated differently than with?

how is this formatted:

object CoreReflection
      extends Foo(
        bar,
        baz
      )
      with scala.meta.internal.trees.Reflection
      with scala.meta.internal.tokens.Reflection

@kitbellew
Copy link
Collaborator

also, even if we consider this, shouldn't we use binPack.parentConstructors to decide whether this is appropriate.

@@ -856,7 +856,9 @@ object TreeOps {
}

def getStartOfTemplateBody(template: Template): Option[Token] =
template.self.tokens.headOption
template.early.headOption
Copy link
Collaborator

Choose a reason for hiding this comment

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

this function must return the token which starts the body of the class/trait/object, not before that

if (template.early.nonEmpty) {
val expire = template.inits.last.tokens.last
Seq(
Split(Space, 0).withIndent(1, expire, ExpiresOn.After),
Copy link
Collaborator

Choose a reason for hiding this comment

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

indent 1? what does this even do?

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.

2 participants