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
fix: bunch of fixes for ScalaCli scripts import #4455
Conversation
50ee78b
to
ce96287
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left several comments/clarification questions, but overall it's LGTM!
metals/src/main/scala/scala/meta/internal/metals/MetalsLanguageServer.scala
Outdated
Show resolved
Hide resolved
metals/src/main/scala/scala/meta/internal/metals/MetalsLanguageServer.scala
Show resolved
Hide resolved
def start(roots: Seq[AbsolutePath]): Future[Unit] = { | ||
ifConnectedOrElse(st => | ||
st.path == path || path.toNIO.startsWith(st.path.toNIO) | ||
)(false) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nit]
/** | |
* @param path a directory or a scalacli script's file path to import | |
*/ |
metals/src/main/scala/scala/meta/internal/metals/scalacli/ScalaCli.scala
Show resolved
Hide resolved
metals/src/main/scala/scala/meta/internal/metals/scalacli/ScalaCli.scala
Show resolved
Hide resolved
metals/src/main/scala/scala/meta/internal/metals/scalacli/ScalaCli.scala
Show resolved
Hide resolved
metals/src/main/scala/scala/meta/internal/metals/scalacli/ScalaCli.scala
Show resolved
Hide resolved
val dir = path.parent | ||
val nioDir = dir.toNIO | ||
val conflictsWithMainBsp = | ||
buildTargets.sourceItems.filter(_.exists).exists { item => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should wait for the indexing here, before indexing, buildTargets.sourceItems
is empty and scalaCliDirOrFile
returns the parent directory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might want to wait for indexing here
metals/metals/src/main/scala/scala/meta/internal/metals/MetalsLanguageServer.scala
Lines 1121 to 1122 in f48987e
if (path.isAmmoniteScript && buildTargets.inverseSources(path).isEmpty) | |
maybeImportScript(path) |
sc
files that belongs to some buildTargets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, we should wait for indexing just in case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thx, good suggestion. Changed!
Fixes scalameta#4447 - use correct semanticdb root for bsp connection. Also don't import a full directory if it conflicts with the sourceItems from the main build tool
ce96287
to
6e7b463
Compare
…eServer.scala Co-authored-by: Rikito Taniguchi <rikiriki1238@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thank you very much @dos65 great job 💯
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Fixes #4447 - use correct semanticdb root for bsp connection.
Also don't import a full directory if it conflicts with the sourceItems from the main build tool