-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Cleanups and source compatibility improvements for repl.AbstractFileClassLoader #24514
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
Merged
+52
−43
Merged
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
40ed8d4
Share common implementation of repl.AbstractFileClassLoader with io.A…
WojciechMazur d0d9717
Add auxilary constructor to repl.AbstractFileClassLoader to preserve …
WojciechMazur 3a4b402
Fix warnings when compiling repl
WojciechMazur a4244e8
Make InterruptInstrumentation typesafe by using enum
WojciechMazur ceca0ec
Replace aux constructor with default value
WojciechMazur 4736f75
Revert "Replace aux constructor with default value"
WojciechMazur a8c2564
Fix compilation o f AbstractFileClassLoaderTest
WojciechMazur File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 think for binary compatibility we would need a separate
def this(...)constructor if I am not mistakenThere 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.
done
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @sjrd for the clarifications. I completely agree with this. T
repland thecompilertoo are not libraries but rather applications, there is no guarantee to have or to follow.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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fully understand this. I will try to vibe code some solution in the background modeled on the presentation compiler interfaces.