hvesalai / emacs-scala-mode

The definitive scala-mode for emacs
http://ensime.org
GNU General Public License v3.0
361 stars 68 forks source link

Indention of lambdas #94

Closed pishen closed 7 years ago

pishen commented 9 years ago

For the code:

seq
  .map { x =>
    val t = x + 1
  }

When I hit enter at the end of x + 1, it become

seq
  .map { x =>
  val t = x + 1

  }

which should be

seq
  .map { x =>
    val t = x + 1

  }
pishen commented 9 years ago

Looks like it only happen when .map is on the second line. (updated)

hvesalai commented 9 years ago

So can you give updated example of what happens and what you expect On 3 Jul 2015 20:05, "Pishen Tsai" notifications@github.com wrote:

Looks like it only happen when .map is on the second line. (updated)

— Reply to this email directly or view it on GitHub https://github.com/hvesalai/scala-mode2/issues/94#issuecomment-118392982 .

pishen commented 9 years ago

I already edited it on the original post https://github.com/hvesalai/scala-mode2/issues/94#issue-92813638

hvesalai commented 9 years ago

This is a bug in scala-mode2. You may work around it by placing the dot on the first line.

seq.
  map { x =>
    val t = x + 1
  }
pishen commented 9 years ago

Also when hitting enter at

seq.map {
  x =>
    val t = x + 1
}

It becomes

seq.map {
  x =>
  val t = x + 1

}

which sould be

seq.map {
  x =>
    val t = x + 1

}

I guess it is from the same problem?

hvesalai commented 9 years ago

Yes, there is currently on support for that either. It has been designed to work as

seq.map { x =>
  val t = x + 1
}

The only support for such is with the case word, i.e.

seq.map {
  case x =>
    val t = x + 1
}
4lex1v commented 7 years ago

I'm suffering from this problem too. Tried to trace the indenting logic, as far as i could understand there are two functions involved into computing the indentation in this case: scala-indent:goto-run-on-anchor and scala-indent:goto-block-anchor. Having the following code:

ws
  .flatMap { response =>

}

The initial point will be before ws rather then the dot before flatMap. Wonder if i should modify an existing rule or add a new one before those i've mentioned above? I'm ready to work on this issue and contribute

hvesalai commented 7 years ago

I think modifying the existing rule is the cleanest solution. I don't know why this bug came to be, but I think it has something to do with the fact that I thought the form is not valid scala. I think I had the impression that the dot must be at the end of the line.

fommil commented 7 years ago

Interesting, yeah this he's started to become pretty idiomatic Scala with all the streaming libraries. I prefer putting short closures on the same line as their inputs TBH.

4lex1v commented 7 years ago

The problem is not with one-line lambdas, but with multiline case deconstructors, e.g:

ws
  .url("...")
  .map(_.getSomething)
  .flatMap {
  case Some(value) => Future.success(value)
  case None => Future.failed(...)
}

Unfortunately examples as above are painful to edit.

I'm looking into fixing this, as far as i understand, scala-indent:apply-indent-rules calls scala-indent:goto-run-on-anchor which receives a point value before w letter, while i guess the fix would be to get the pointer value of the . before flatMap in the snippet above.

hvesalai commented 7 years ago

Yes, this is how I format it and how I tested it when writing the indentation code

ws.
  url("...").
  map(_.getSomething).
  flatMap {
    case Some(value) => Future.success(value)
    case None => Future.failed(...)
  }

if we want @4lex1v 's version to work, then the ~scala-indent:goto-run-on-anchor~ scala-indent:goto-run-on-anchor needs to be changed so it stops at the . if it follows a valid nl. The rule for a valid nl is defined in https://www.scala-lang.org/files/archive/spec/2.11/01-lexical-syntax.html#newline-characters.

hvesalai commented 7 years ago

(comment redacted as utter bollocks)

hvesalai commented 7 years ago

Ok, so an other take: https://github.com/ensime/emacs-scala-mode/blob/master/scala-mode-indent.el#L600

Here we skip back sexps until we reach a beginning of line. Since . is ignored as punctuation when skipping sexps, it skips from the point before flatMap (but after .) to the beginning of map (but after .) and so forth until it reaches point before ws which is at the beginning of line.

4lex1v commented 7 years ago

@hvesalai how hard would it be to support both indentation schemes? Would it require some sort of defcustom variable or changes to the algorithm would be enough?

hvesalai commented 7 years ago

This is also broken:

foo(foo
      ,bar)
hvesalai commented 7 years ago

@4lex1v if I fix it, will you test it?

4lex1v commented 7 years ago

@hvesalai yes

hvesalai commented 7 years ago

And by testing we mean that it should be checked that it doesn't break anything outside what it means to fix. So please reindent large amounts of code with this.

4lex1v commented 7 years ago

@hvesalai The fix for my case works, i'll test it more today, thank you. Though your examples foo(foo, bar) look like this:

object test {
  foo(foo
    ,bar)
}

which is a bit different

hvesalai commented 7 years ago

Yes, that is a different rule (parameter alignment)