improvement: sorting workspace members with same name by frequency #6393
Conversation
Temurlock
force-pushed
the
sorting_workspace_members
branch
2 times, most recently
from
c19e31b
to
b764a3a
Compare
| if (!visited(sourcePath)) { | ||
| visited.add(sourcePath) | ||
| sourcePath.exists | ||
| } else false |
There was a problem hiding this comment.
| if (!visited(sourcePath)) { | |
| visited.add(sourcePath) | |
| sourcePath.exists | |
| } else false | |
| if (visited.add(sourcePath)) { | |
| sourcePath.exists | |
| } else false |
This could be simplified to check whether the given element already belongs to the set only once. 🙂
There was a problem hiding this comment.
Thanks, made it shorter: visited.add(sourcePath) && sourcePath.exists
Temurlock
force-pushed
the
sorting_workspace_members
branch
from
b764a3a
to
bf42d0d
Compare
| @@ -266,6 +266,8 @@ public abstract PresentationCompiler withScheduledExecutorService( | |||
| */ | |||
| public abstract PresentationCompiler withWorkspace(Path workspace); | |||
|
|
|||
| public abstract PresentationCompiler withReferenceCounter(ReferenceCountProvider provider); | |||
There was a problem hiding this comment.
We also use old mtags for scala versions that aren't supported anymore, so each new added method needs a default implementation. This is the reason for most of the CI fail.
| Seq( | ||
| if (scalaVersion.startsWith("2.13")) | ||
| Dependency.of(Module.of("org.typelevel", s"cats-effect_2.13"), "3.5.3") | ||
| else if (isScala3Version(scalaVersion)) | ||
| Dependency.of(Module.of("org.typelevel", s"cats-effect_3"), "3.5.3") | ||
| else | ||
| Dependency.of(Module.of("org.typelevel", s"cats-effect_2.12"), "3.5.3") | ||
| ) |
There was a problem hiding this comment.
Wouldn't it be enough to use binaryVersion instead?
| else | ||
| completion.getItems.asScala | ||
| .sortBy(_.getLabel()) | ||
| .map(item => fullServer.completionItemResolve(item).get()) |
There was a problem hiding this comment.
NIT: maybe move the common line out of if statement.
| referenceCache | ||
| .getOrElseUpdate( | ||
| w2.sym, | ||
| referenceCounter | ||
| .references(buildTargetIdentifier, semanticdbSymbol(w2.sym)) | ||
| ) |
There was a problem hiding this comment.
NIT: maybe extract this part
|
|
||
| public interface ReferenceCountProvider { | ||
|
|
||
| Integer references(String buildTargetIdentifier, String symbol); |
There was a problem hiding this comment.
I wonder whether we should make it more generic. Like workspacePriority which we could modify on Metals side.
Temurlock
force-pushed
the
sorting_workspace_members
branch
from
bf42d0d
to
3e1f2f3
Compare
Temurlock
force-pushed
the
sorting_workspace_members
branch
from
2d23ffe
to
e67d5d3
Compare
|
|
||
| public interface CompletionItemPriority { | ||
|
|
||
| Integer workspaceMemberPriority(String buildTargetIdentifier, String symbol); |
There was a problem hiding this comment.
Let's add docs what is expected by returning an integer here. What happens when the number is lower and higher.
| config: PresentationCompilerConfig = PresentationCompilerConfigImpl(), | ||
| folderPath: Option[Path] = None, | ||
| reportsLevel: ReportLevel = ReportLevel.Info, | ||
| buildTargetIdentifier: String = "", |
There was a problem hiding this comment.
You probably need to run scalafmt via ./bin/scalafmt
| @@ -192,6 +192,19 @@ trait Completions { this: MetalsGlobal => | |||
| new Ordering[Member] { | |||
| val queryLower = query.toLowerCase() | |||
| val fuzzyCache = mutable.Map.empty[Symbol, Int] | |||
| val workspaceMemberPriorityCache = mutable.Map.empty[Symbol, Int] | |||
There was a problem hiding this comment.
Will this cache be properly refreshed when the frequency changes? (the tests seem to indicate so) Maybe we should control and cache it in ReferenceProvider? For example every time semanticdb changes we could refresh the cache for all symbols contained in the TextDocument? So it would not cause any additional time for completions?
| val futureCompletionResult: List[String] = | ||
| List("Future - scala.concurrent", "Future - java.util.concurrent") | ||
|
|
||
| checkItems( |
There was a problem hiding this comment.
Why not use check it would still be visible which one is first.
inspired by: scalameta/metals-feature-requests#254 (comment)
Added ranking by frequency for workspace symbols with the same name.
It useful when project has multiple objects with same name (like Future (scala, java, twitter) or Clock (java, cats, zio))
It uses ReferenceProvider to count files where workspace member could have been referenced
False positive result from bloom filter is counted as reference in file to keep sorting fast (if we wanted to make it precise we should read semanticdb of matching paths which is expensive)