Cleanups and source compatibility improvements for repl.AbstractFileClassLoader #24514
Conversation
…bstractFileClassLoader
…source compatibilty
WojciechMazur
added
the
backport:nominated
| root: AbstractFile, | ||
| parent: ClassLoader, | ||
| interruptInstrumentation: InterruptInstrumentation = InterruptInstrumentation.fromString(ScalaSettings.XreplInterruptInstrumentation.default) | ||
| ) extends io.AbstractFileClassLoader(root, parent): |
There was a problem hiding this comment.
I think for binary compatibility we would need a separate def this(...) constructor if I am not mistaken
There was a problem hiding this comment.
There's no binary compatibility to uphold. We shouldn't worsen our code to "preserve" binary compatibility.
Even preserving source compat is a benevolent favor we make. We can do it if it helps other projects, but not to expense of our own code quality.
There was a problem hiding this comment.
This has been stable for a while and we use it in Mdoc, which then is used by worksheets in Metals. We don't fully cross publish for different Scala versions, we just check if each version works. I don't really want to add another library that we would need to cross publish.
The other solution would be to have a worksheet API, but that is something no one had the time for.
There was a problem hiding this comment.
https://github.com/scalameta/mdoc/blob/main/mdoc/src/main/scala-3/mdoc/internal/markdown/MarkdownCompiler.scala this is the file that uses it.
There was a problem hiding this comment.
Thanks @sjrd for the clarifications. I completely agree with this. T repl and the compiler too are not libraries but rather applications, there is no guarantee to have or to follow.
There was a problem hiding this comment.
Sure, we should think of having an interface for worksheets, but if we decide not to include this change, worksheets will stop working altogether and it will require a lot of my work to fix it. At a cost of a single added custom constructor.
There was a problem hiding this comment.
Also, I think we already accepted much worse code that is being fixed by Wojtek in this PR, so the change is anyway a net positive. Not sure why we would be blocking this change (which will save me loads of time) and anyway accept much worse PRs. :/
There was a problem hiding this comment.
I'm not blocking anything. But the expectation for this code should be that it will break. There is no guarantee here.
There was a problem hiding this comment.
I fully understand this. I will try to vibe code some solution in the background modeled on the presentation compiler interfaces.
4 participants
REPL and AbstractFileClassLoader is used by multiple downstream dependencies (eg. scalameta/mdoc), this PR is oriented on improving source compatibility of this class :
io.AbstractFileClassLoaderandrepl.AbstractFileClassLoaderinterruptInstrumentationconstructor arguemntInterruptInstrumentationan enum