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

SortModifiers does not follow Scala style guide #3788

Closed
dagmendez opened this issue Feb 20, 2024 · 3 comments · Fixed by #3797
Closed

SortModifiers does not follow Scala style guide #3788

dagmendez opened this issue Feb 20, 2024 · 3 comments · Fixed by #3797

Comments

@dagmendez
Copy link

The Scala Style Guide suggest the following:

Modifiers
Method modifiers should be given in the following order (when each is applicable):

  1. Annotations, each on their own line
  2. Override modifier (override)
  3. Access modifier (protected, private)
  4. Implicit modifier (implicit)
  5. Final modifier (final)
  6. def
@Transaction
@throws(classOf[IOException])
override protected final def foo(): Unit = {
 ...
}

Is the current sorting of the modifiers correct and the Scala Style Guide is outdated or is it the other way around?

Configuration (required)

version = 3.8.0 - main branch
...
object SortSettings {
  final case class ModKey(
      name: String,
      matches: Mod => Boolean
  )

  val defaultOrder: List[ModKey] = List(
    ModKey("implicit", _.is[Mod.Implicit]),
    //
    ModKey("final", _.is[Mod.Final]),
    ModKey("sealed", _.is[Mod.Sealed]),
    ModKey("abstract", _.is[Mod.Abstract]),
    //
    ModKey("override", _.is[Mod.Override]),
    //
    ModKey("private", _.is[Mod.Private]),
    ModKey("protected", _.is[Mod.Protected]),
    //
    ModKey("lazy", _.is[Mod.Lazy]),
    ModKey("open", _.is[Mod.Open]),
    ModKey("transparent", _.is[Mod.Transparent]),
    ModKey("inline", _.is[Mod.Inline]),
    ModKey("infix", _.is[Mod.Infix]),
    ModKey("opaque", _.is[Mod.Opaque])
  )

Command-line parameters (required)

When I run scalafmt via CLI like this: scalafmtAll

Steps

Given code like this:

override protected final def foo(): Unit = {
  ...
}

Problem

Scalafmt formats code like this:

final override protected def foo(): Unit = {
  ...
}

Expectation

I would like the formatted output to look like this:

override protected final def foo(): Unit = {
@kitbellew
Copy link
Collaborator

feel free to modify the parameters of this rule the way you prefer. we can't change the default as that will likely affect too many existing users.

@rubiesonthesky
Copy link

I think it would be good idea to add note to documentation that the current default does not follow scala style guide and how to set it so that it will follow it.

Is this something that you would accept PR for?

And maybe this could be changed in next major version?

@dagmendez
Copy link
Author

@kitbellew That is why I have open this issue. We have already modify the order in our project by overriding the rule.

rewrite.sortModifiers.order = [
  sealed
  abstract
  override
  private
  protected
  implicit
  final
  lazy
  open
  transparent
  inline
  infix
  opaque
]

The thing is that the Scala Style Guide does not cover all the modifiers while talking about the order of them (from the Scala 3 Reference)

  • ‘override’
  • ‘opaque’
  • ‘abstract’
  • ‘final’
  • ‘sealed’
  • ‘open’
  • ‘implicit’
  • ‘lazy’
  • ‘inline’
  • ‘transparent’
  • ‘infix’
  • ‘private’
  • ‘protected’

I think the core of the discussion in this issue should be to clarify if there is an actual agreement of the order of all the modifiers. This issue already bring the attention to the fact that there is a slight divergence among the Style Guide and the defaults.

From my point of view, a warning should be added in the documentation that the defaults do not follow the Scala Style Guide. What do you think?

In the meantime, we can reach to the Scala Style Guide writers and ask them for a comprehensive order for the modifiers.

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 a pull request may close this issue.

3 participants