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

Introduce scala-indent:defition-parameter-scaling-factor #173

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mzhaom
Copy link

@mzhaom mzhaom commented Oct 26, 2021

to control whether the parameter list of function/class should have a
different indentation level.

See discussion in #172

control whether the parameter list of function/class should have a
different indentation level.

See discussion in hvesalai#172
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

(save-excursion
(goto-char anchor)
(when
(or (string= "class" (current-word))
Copy link
Owner

Choose a reason for hiding this comment

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

Can you fix the indentation here. You are most probably using tabs, please use spaces only.

(or (string= "class" (current-word))
(string= "def" (current-word)))
;; try to find (
(while (and (< (point) start)
Copy link
Owner

Choose a reason for hiding this comment

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

I'm not quite sure what you are trying to achieve here. I read the code as follows: you search for (, skip it, then search for ), skip that. Why?

Copy link
Author

Choose a reason for hiding this comment

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

What I was trying to do is:

  • Confirm I can see "(", so we are inside parameter list.
  • Confirm I couldn't see ")" beyond that, so we are not outside parameter list.

I'm not sure whether there is some assumption I can leverage so simplify that, happy to take them.

Also the approach doesn't really work for case like this because there is more complicated scenario like annoation within for the parameter list, like this

case class Option(
     @arg(
        name="port",
        doc="TCP port to listen to"
     )
     port: Int = 0,
)

Copy link
Owner

Choose a reason for hiding this comment

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

Ok... first some note:

  • you can't be searching for ) because you can't expect the code to be complete by the time you already need to indent it. Hence indentation should never look beyond the line that is being indented.

  • you can't expect the definition keyword to be after anchor. For example, the case

def foo
  (
    bar: Int // anchor for this is at (, not the line with def
  )

(you could of course ignore this for your specific modification)

  • you should take into account that both def's and constructors can have multiple parameter lists.

But then some implementation hints:

  1. check out https://github.com/hvesalai/emacs-scala-mode/blob/master/scala-mode-indent.el#L626

here (nth 1 (syntax-ppss start)) gives The character position of the start of the innermost parenthetical grouping containing the stopping point; nil if none. stopping point being start. You can use this to jump to the opening brace to check that it is (.

  1. after checking for ( you are now outside the parenthesis, so you can use backward-sexp to jump over preceding lists (type lists and parameter lists) and the name to arrive at the keyword.

As an example, consider the following code

private class Foo[X <: List[_]](implicit ev: Evidence)(
    x: String,
    y: Int

Let's assume start is at y on the last line and so anchor woudl be at p on the first line.

Now if you follow the instruction 1 above, you can use that to jump out of the parameter list, i.e. to the last ( on the first line. You check that it is indeed ( and if so, then you use backward-sexp to jump over all lists (i.e. jump as long as you land at (, or [). Now you would be at F. You then jump once more using backward-sexp and you should be at c on the first line. You would then use (looking-at-p "class\\|def") to check that you were indeed inside a definition list.

That's it.

HAVING SAID THAT, now that I read the code, I see scala-syntax:beginning-of-definition, which sounds like exactly what you want. However, it jumps to the start of the line, not the definition work. We should refactor the code a bit to get it to do what you need.

I propose you implement the thing without refactoring scala-syntax:beginning-of-definition to see that the approach actually works, and then I can refactor the code before I merge it.

Copy link
Owner

Choose a reason for hiding this comment

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

Actually the thing I did with syntax-ppss can also be done with backward-up-list, but then you have to handle the error it throws if you are NOT in the list. I think that's why I've done it the low-level way

@hvesalai
Copy link
Owner

Thank you for the PR. It looks promising, but I would like to understand it better first.

It seems you want to only indent definition lists, but not function call lists. Why is this?

def foo(s: String, b: Int)

foo(
    "shouldn't this row have an indent of four spaces compared to the function name?",
    0
)

@mzhaom
Copy link
Author

mzhaom commented Oct 26, 2021

Thank you for the PR. It looks promising, but I would like to understand it better first.

It seems you want to only indent definition lists, but not function call lists. Why is this?

def foo(s: String, b: Int)

foo(
    "shouldn't this row have an indent of four spaces compared to the function name?",
    0
)

Yeah, that's the databricks scala style, I don't know why the chose so.
https://github.com/databricks/scala-style-guide#indent
I'm still thinking about how to handle extends and with after class parameter list.

For classes whose header doesn't fit in two lines, use 4 space indentation for its parameters, put each in each line, put the extends on the next line with 2 space indent, and add a blank line after class header.

class Foo(
    val param1: String,  // 4 space indent for parameters
    val param2: String,
    val param3: Array[Byte])
  extends FooInterface  // 2 space indent here
  with Logging {

  def firstMethod(): Unit = { ... }  // blank line above
}

For method and class constructor invocations, use 2 space indentation for its parameters and put each in each line when the parameters don't fit in two lines.

foo(
  someVeryLongFieldName,  // 2 space indent here
  andAnotherVeryLongFieldName,
  "this is a string",
  3.1415)

new Bar(
  someVeryLongFieldName,  // 2 space indent here
  andAnotherVeryLongFieldName,
  "this is a string",
  3.1415)

@hvesalai
Copy link
Owner

@mzhaom did you have a chance to look at my other comments

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.

4 participants