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

Handle abstract var overrides #1056

Merged
merged 1 commit into from Nov 10, 2019
Merged

Handle abstract var overrides #1056

merged 1 commit into from Nov 10, 2019

Conversation

@gabro
Copy link
Member

gabro commented Nov 10, 2019

Fixes #1029

Previously we would prevent overriding var members altogether, recently there seems to be been a regression where we suggest def completions for var members (#1029); now we allow overriding abstract var members, while still rejecting concrete var overrides.

@@ -452,7 +452,7 @@ trait Completions { this: MetalsGlobal =>
pos,
text,
valdef.pos.start,
_.isStable
_ => true

This comment has been minimized.

Copy link
@gabro

gabro Nov 10, 2019

Author Member

vars are not stable symbols, but we can still override them in some cases

@gabro gabro force-pushed the gabro:fix-1029 branch from f710d77 to cfc8b62 Nov 10, 2019
@ckipp01

This comment has been minimized.

Copy link
Member

ckipp01 commented Nov 10, 2019

Awesome! This is exactly what I needed! I just locally rebased on this and my issues with #1031 go away entirely and all tests pass for 2.11, 2.12, and 2.13. Thanks for looking at this 👏

| override val hello@@
}
|""".stripMargin,
// NOTE(gabro): we inlcude also var and def completions despite the user typed a val

This comment has been minimized.

Copy link
@gabro

gabro Nov 10, 2019

Author Member

I honestly couldn't make this work, but it's not too terrible as UX

This comment has been minimized.

Copy link
@olafurpg

olafurpg Nov 10, 2019

Member

I think this UX is totally fine, you don't always know if the supermethod is a def, val or a var

@@ -1087,7 +1094,7 @@ trait Completions { this: MetalsGlobal =>
!sym.isEffectivelyFinal &&
!sym.name.endsWith(CURSOR) &&
!sym.isConstructor &&
!sym.isMutable &&
(!isVarSetter(sym) || (isVarSetter(sym) && sym.isAbstract)) &&

This comment has been minimized.

Copy link
@gabro

gabro Nov 10, 2019

Author Member

allow overriding vars, but only if they're abstract

@gabro gabro requested review from olafurpg and tgodzik Nov 10, 2019
Copy link
Member

olafurpg left a comment

Very nice! Thank you @gabro

I didn't know you could override abstract vars. When I first implemented "override def" completions I only tried overriding concrete vars and assumed it wasn't supported

| override val hello@@
}
|""".stripMargin,
// NOTE(gabro): we inlcude also var and def completions despite the user typed a val

This comment has been minimized.

Copy link
@olafurpg

olafurpg Nov 10, 2019

Member

I think this UX is totally fine, you don't always know if the supermethod is a def, val or a var

@olafurpg olafurpg merged commit f54ca49 into scalameta:master Nov 10, 2019
9 checks passed
9 checks passed
Windows unit tests
Details
Linux unit tests
Details
Sbt integration
Details
Maven integration
Details
Gradle integration
Details
Mill integration
Details
Slow tests
Details
Scala cross tests
Details
Scalafmt/Scalacheck/Docs
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.