Should Metals insert override keyword when overriding abstract method?

[olafurpg]

When overriding an abstract method, the override keyword is optional:

trait Trait {
  def method: Int
}

class Main extends Trait {
  def method<COMPLETE>
  // option 1: override def method: Int = ???
  // option 2: def method: Int = ???
}

Each option has upsides and downsides, see discussion from #540 (comment)

I’d like to add that I’m annoyed by the IntelliJ behavior that always inserts the override keyword even when the code overrides nothing but implements an abstract member. I think this is a bad practice because if that member becomes non-abstract in a future version your code will have no clue that it is now overriding a default implementation (which may not be the initial intent). Therefore I would suggest not inserting the override keyword in case the member is abstract. - @julienrf

Ah interesting @julienrf, we actually have the opposite policy at work: we add override to make sure you get meaningful errors if the abstract member changes signature.
If you don't, you get a giant error on the entire class implementation, but if you add override, the error stays local and you spot it quickly. - @gabro

@gabro If the abstract member changes signature then you also get an error: “missing unimplemented members…” - @julienrf

I personally lean towards always inserting override because I find that easier to read and it has the benefit that you get a compile error "method foo does not override anything" in case the supermethod changes a signature gets removed.

[olafurpg]

On principle, we should avoid configuration options as much as possible so supporting both is not a good solution IMO. My experience with Scalafmt is that configuration options make it significantly harder to maintain and evolve a project.

[olafurpg]

Here is a proposal:

• always insert the override keyword when overriding concrete methods, this is required for the code to compile.
• when overriding an abstract method, never insert insert override but leave it unchanged if the user writes override def method<COMPLETE>.

[gabro]

Fine by me! I value not having config much more than fitting my specific workflow :)

[japgolly]

There are two trains of thought in this but by choosing a compromise solution and not giving people control, means that both camps are going to be eternally unhappy half of the time (half I mean because if I want overrides all the time, I get them in 1 of the 2 scenarios). Is that worse than having a bit of config? In my opinion it is and I can see my self getting annoyed and that unhappiness growing every day that it frustrates me. Is there another way and/or can we consider having config? For future features too, I imagine that if metals makes a hardcover decision about what people want at each fork in the road of future features, unhappiness will increase more and more and eventually be that opinionated for a significant number of people.

…

On Thu., 14 Mar. 2019, 8:12 pm Gabriele Petronella, < ***@***.***> wrote: Fine by me! I value not having config much more over my specific workflow :) — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#565 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAMYt35VTRapX--KQpdkXikwPCQ8EDo5ks5vWhKEgaJpZM4bzh71> .

[olafurpg]

@japgolly Thanks for the input! I believe there is room for a Scalafix rule or some tool to automatically enforce that override is used everywhere where applicable. A lot of thought has gone into making Scalafix configurable via .scalafix.conf and Scalafmt via .scalafmt.conf and I would prefer to avoid introducing .metals.conf if possible.

[olafurpg]

The PR #570 implements the proposal from #565 (comment)

Users who always want override can write override def <COMPLETE> and the users who don't want override when implementing an abstract method can write def <COMPLETE>.