-
Notifications
You must be signed in to change notification settings - Fork 120
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
Deprecate ScalaThenJava, and improve the docs #889
base: develop
Are you sure you want to change the base?
Conversation
To validate this pull request against other sbt modules, please visit this link. |
15ba7c2
to
435181f
Compare
My reading of #235 is that there's a small but not insignificant difference. If there isn't a strong reason to drop this I'd say let's avoid the needless breakage. |
Insignificant difference in the promised meaning between |
You can't make the Scala source depend on the Java source because the Java source is never passed to scalac. The fact that it's not handled in incremental is an incremental bug. But people might still be relying on it for regular, full compilation (e.g. in CI), so I don't think it should be just deleted. |
I'm happy to update the documentation to reflect the reality as much as possible, but I think this is a good candidate to deprecate it as an option since it's a pointless ordering that would only cause confusion. |
435181f
to
0aeced2
Compare
To clarify the situation, do you agree with my assessment that all these |
That's not my assessment, no. My guess is that people use it for either of those reasons and I wouldn't characterise it as a "side effect" or "downside" - it's a strategy. |
Does anyone use |
Those are just guesses, right? Are these all instances of confusion? https://github.com/search?p=4&q=ScalaThenJava+extension%3Asbt&type=Code
What are you basing that on? Looks like it's even documented: https://www.scala-sbt.org/1.x/docs/Java-Sources.html |
re: I don't think anyone actually uses this flag
I did search GitHub before commenting the above, and yes, compared to 200k builds out there one effectively no active project is using re: CompileOrders are there to squeeze out scalac-java-parse performanceNice find on the documentation. I think the documentation is consistent with what I was saying, which is the the purpose of this flag is to eke out performance:
I think it would be good to test this claim too. Maybe it's still relevant when you have 100s of generated Java sources. re: implementation detail / weird hack at best.I expect Zinc to act as a non-opinionated wrapper around Scala compiler, Java compiler, and we should refrain from adding semantic changes to how the code is compiled. |
Btw, scala/scala depends on JavaThenScala: https://github.com/scala/scala/blob/8329196f299cc16946adc3f9d03a408441df6a96/build.sbt#L442-L444 |
I have mixed, mostly-negative feelings about deprecating Finally — and this argument only occurred to me as I was writing the above paragraph — Scala's Java parser hasn't been updated in donkey's years. There are post-Java-8 constructs that it doesn't understand. If my Java code uses those constructs, I'm going to need (I wouldn't miss |
ok. Let's keep |
@dwijnand you never weighed back in about deprecating |
0aeced2
to
1d26dbe
Compare
Ref sbt#235 Co-authored-by: Seth Tisue <[email protected]>
1d26dbe
to
3abefcb
Compare
note the introduction of |
I'm just as against deprecating is as JavaThenScala. The costs outweighs the benefits, in my opinion: it's going to introduce noise, thus more work, possibly fatal warning breakages, or possible real breakages when the deprecation cycle is done and its removed, and what have we gained? It might not be the most popular feature , but removing it isn't worth it, as I see it. |
I'm not super gung ho about merging this, so whatever @eed3si9n decides is fine w/ me. I worry about the maintenance footprint (for us) and the mental bandwidth footprint (for users) of little-used, under-tested features, but I don't have much background in how such decisions tend to play out in this repo or in sbt generally, and Dale's position makes sense too, so... 🤷 |
Setting aside the deprecation debate, we should document:
|
re: 1 and 2: Okay — I could review further documentation improvements, but I'm not a position to make them myself.
re: 3, the current state of the PR does that to the best of my ability, except that I spent less time on |
I've no qualms at all in recording the behaviour details. |
Ref #235