Scaladoc: Introduce new variables to create better links to source #7532
Conversation
| val filePath = fixPath(file.path).replaceFirst("^" + assumedSourceRoot, "").stripSuffix(".scala") | ||
| inSource map { case (file, line) => | ||
| val filePathExt = fixPath(file.path).replaceFirst("^" + assumedSourceRoot, "") | ||
| val filePath = filePathExt.stripSuffix(".scala") |
There was a problem hiding this comment.
shouldn't this be something like filePathExt.stripSuffix(".scala").stripSuffix(".java") to fix scala/bug#5388?
There was a problem hiding this comment.
No, the existing use is as shown in https://github.com/scala/scala/pull/7532/files#diff-fdc3abdfd754eeb24090dbd90aeec2ceL164 where the .scala is added by the URL template. Removing even the .java doesn't help and would make the current fix scala/bug#5388 (comment) via post-processing impossible.
There was a problem hiding this comment.
I see, but it still seems strange to me to remain bug-compatible here. Your documentation says "€{FILE_PATH} gives scala/collection/Seq", but that's only true for .scala files.
How about providing €{FILE_PATH} and €{FILE_EXT}, where "ext" is any file extension, and "path" is without that extension. Something like
val (filePath, fileExt) = filePathExt.splitAt(filePathExt.indexOf(".", filePathExt.lastIndexOf("/")))
There was a problem hiding this comment.
I see what you mean, the documentation for FILE_PATH is still wrong for non-Scala files.
I think it is a bad idea to change the meaning of FILE_PATH.
What about removing FILE_PATH from the documentation, but leaving it in the code for backwards-compatibility?
There was a problem hiding this comment.
I think the current FILE_PATH is buggy and we should just fix it. It would stay the same for scala files, which is the majority. Yes, the workaround would break, but I doubt a lot of people are doing this.
There was a problem hiding this comment.
I fixed FILE_PATH to never include the extension, added FILE_EXT, and left FILE_PATH_EXT to get the whole thing.
There was a problem hiding this comment.
Thanks, I was stubborn enough :-)
|
A test case would be nice.. https://github.com/scala/scala/tree/2.13.x/test/scaladoc/run should have some examples. Maybe @janekdb could point at a good one? There are some |
|
Sure. You don't trust Github squashing? |
Introduces new variables for -doc-source-url
FILE_PATH_EXT - same as FILE_PATH, but including the file extension (which might be .java)
FILE_EXT - the file extension (.scala or .java)
FILE_LINE - containing the line number of the Symbol
Fixes FILE_PATH to never contain the file extension (see scala/bug#5388)
ennru
force-pushed
the
extension-in-doc-source-url
branch
from
4333ae1
to
0379a26
Compare
|
We want the merge commits, for example we run benchmarks on all merge commits. I use it from time to time, but generally we don't. What annoys me most is that once you do "Squash and merge", the button changes its default, so you have to be careful to switch back when you merge the next PR. |
|
Do you think parts of this could go into 2.12, as well? |
|
Yes, this can be backported as-is I'd say. |
Introduces new variables for
-doc-source-urlFILE_PATH_EXT- same asFILE_PATH, but including the file extension (which might be.java)FILE_LINE- containing the line number of the SymbolFixes scala/bug#5388