let AbsolutePath do the Uri encoding

[martijnhoekstra]

on Windows, more than just prepending file: is needed

[olafurpg]

LGTM 👍

[laughedelic]

Good 👍 Does it fix some failures in #276?

[martijnhoekstra]

It locally fixes some bugs in #276 - maybe all URI bugs - but CI isn't positive. I probably need to use your CI as my unix test env.

[olafurpg]

Heads up on #275, it touches a big chunk of the codebase.

The test failures may be cause by the implicit requirement that those uris need to be formatted as file:/ with single slash instead of ///.

[martijnhoekstra]

metals/metals/src/main/scala/scala/meta/metals/Uri.scala

Line 18 in 00483bd

* - Both file:///path and file:/path are valid URIs while we only use

seems to suggest the opposite - is that a typo?

[laughedelic]

That comment is correct (also see the linked issue) and without your change I see in my local logs file:/// absolute paths. So the issue should be somewhere else.

[olafurpg]

I thought as well that all URIs should be file:/// but my repl says Semanticdbs.scala was using another convention

@ Paths.get("").toAbsolutePath.toUri
res1: java.net.URI = file:///Users/ollie/dev/metals/

@ "file:" + Paths.get("").toAbsolutePath
res2: String = "file:/Users/ollie/dev/metals"

[laughedelic]

@martijnhoekstra it fails because the same hack is hardcoded in the tests 😬

metals/metals/src/test/scala/tests/search/SymbolIndexTest.scala

Line 144 in 8b2e169

s"file:${path.toString}",

So you need to change it there too.

UPD: no, I guess it's not the reason either. There are several places in the code doing these conversions in different way. At some point scala.meta.metals.Uri was introduced to unify it.

[olafurpg]

Good catch @laughedelic !

Uri was intended to unify all usages of URIs in the codebase, so that what we should be using here. java.net.URI is not used because they do not have structural equality in their string form.

[olafurpg]

Let's do Uri(absolute).value and then fix the broken implementation of Uri.apply

  def apply(uri: URI): Uri =
    new Uri(uri.toString) {}

This part here

    if (uri.getScheme == "file") {
      // nio.Path.toUri.toString produces file:/// while LSP expected file:/
      new Uri(s"file://${uri.getPath}") {}

is broken since URI.getPath looses the URI encoding 🤦‍♂️

[olafurpg]

Good catch @laughedelic !

Uri was intended to unify all usages of URIs in the codebase, so that what we should be using here. java.net.URI is not used because they do not have structural equality in their string form.

prematurely accepted

[martijnhoekstra]

I can't push this to completion today, unfortunately. Will do so tomorrow.

[laughedelic]

@martijnhoekstra no problem. It's even better if you do it after the big refactoring in #275 is merged.

[gabro]

@martijnhoekstra AppVeyor landed on master, you can rebase if you want to check that the CI is happy too.

[martijnhoekstra]

@gabro It might be better if I wait on #275 and rebase when that lands. I'll also enable CI for all tests that pass after rebasing.

[gabro]

👍 sounds like a plan

[olafurpg]

Superseded by #275

I hit on this bug while making the test suite pass