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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

SortModifiers rewrite fixes + doc prettyfying #1153

Merged
merged 6 commits into from Apr 24, 2018

Conversation

2 participants
@lorandszakacs
Contributor

lorandszakacs commented Apr 24, 2018

Closes #1148 (bug), #1149 (docs)

There is one super weird corner case that is not handled properly by the fix. There's also a test for it:
If you have a sort order where implicit has lower order than the rest, then the following code gets formatted weirdly.

Config:

rewrite {
  rules = [SortModifiers]
  sortModifiers {
    order = ["private", "protected" , "abstract", "final", "sealed", "implicit", "override", "lazy"]
  }
}

Original code:

class X(private final val x: Int)(
    implicit final private val i1: Int,
    implicit final private var i2: Int
)

Formatted code:

class X(private final val x: Int)(
    implicit private final val i1: Int,
    private final implicit var i2: Int
)

But this is a super weird corner case where the implicit keyword is duplicated, and honestly until I wrote this format I had no idea that was even possible 馃槄.

You will also notice that this implementation is actually cleaner than the buggy one, which is a pleasant surprise 馃槂
/cc @olafurpg

@olafurpg

One minor comment, otherwise looks good. Thanks for the quick fix!

* and apply patches
*/
private def isHiddenImplicit(m: Mod): Boolean = {
m.tokens.isEmpty && m.is[Mod.Implicit]

This comment has been minimized.

@olafurpg

olafurpg Apr 24, 2018

Member

I think this will return true for def foo(implicit x: Int) due to a bug, can you double check?

This comment has been minimized.

@lorandszakacs

This comment has been minimized.

@lorandszakacs

lorandszakacs Apr 24, 2018

Contributor

@olafurpg indeed it does return true, but luckily no rewrite is necessary in this case, so the final result is OK. Added an explicit test case for this:

<<< 1148 鈥 only implicit param
def foo(implicit x: Int)
>>>
def foo(implicit x: Int)
@olafurpg

LGTM 馃憤

@olafurpg olafurpg merged commit 7157f1d into scalameta:master Apr 24, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@olafurpg

This comment has been minimized.

Member

olafurpg commented Apr 24, 2018

This came out nice 馃憤

screen shot 2018-04-24 at 13 37 02

@olafurpg

This comment has been minimized.

Member

olafurpg commented Apr 24, 2018

I'll try to get a hotfix out later this week, I want to address #1147 as well

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment