Deprecates Extracted#append for appendWithSession#3865
Conversation
|
The test I wonder if this is spurious? I'll rerun Travis CI now that I've copy/pasted the failing output. |
| val appendSettings = | ||
| Load.transformSettings(Load.projectScope(currentRef), currentRef.build, rootProject, settings) | ||
| val newStructure = Load.reapply(session.original ++ appendSettings, structure) | ||
| val newStructure = Load.reapply(session.mergeSettings ++ appendSettings, structure) |
There was a problem hiding this comment.
Instead changing append's behaviour, add another method (or 2) which has well defined semantics.
f808746 to
1f8e286
Compare
1f8e286 to
f1a6b77
Compare
|
@eed3si9n WDYT now? |
| display.show(ScopedKey(scope, key)) + " is undefined.") | ||
|
|
||
| def append(settings: Seq[Setting[_]], state: State): State = { | ||
| /** Appends the given settings to the original build state settings. */ |
There was a problem hiding this comment.
I suggest adding information about the gotcha of discarding session settings, perhaps marking this as deprecated
There was a problem hiding this comment.
I think being more explicit about session settings makes sense.
But if we're assuming that discarding them is intended behaviour and a valid use case, we can't mark this as deprecated.
There was a problem hiding this comment.
It wasn't exactly intended behavior, at least originally. The method append, with the current semantics, should have been deprecated long ago. As a reference:
On 06/03/13 02:27, Mark Harrah wrote:
On 06/03/13 01:34, Antonio Cunei wrote:
However, I guess Extracted.append() could mirror more closely the
operation of the set command, otherwise the result may be surprising to
users. (Also, an equivalent to "set every" might be handy). Is this
analysis correct, or is there a different recommended way to patch the
session?Your understanding is correct. There are different semantics that are useful at different times. I'm not real sure if it is the right thing for commands to effectively do the same as set. They don't tend to be saveable since there isn't a corresponding source. One way forward is to deprecate append and replace it with two (or more) methods with documented semantics.
There was a problem hiding this comment.
Maybe we should deprecate in favour of appendWithSession and appendDiscardingSession?
[edit: or appendWithoutSession]
There was a problem hiding this comment.
Not that historical text by Mark should be taken as sacred, if I am reading this right he's saying that while he was unsure if it's the right thing, he was discarding intentionally and thus suggesting we have "two (or more) methods".
Here's my suggestion:
- Add
appendWithoutSession(settings: Seq[Setting[_]], state: State)andappendWithSession(settings: Seq[Setting[_]], state: State). - Deprecate
append(...)explaining that theappenddiscards session, and that the user is encourage to move to eitherappendWithSession(...)orappendWithoutSession(...).
I am also tempted to add an alias called set(...) for appendWithSession(...) as it would have the same semantics as the set command, but I'm afraid append vs set would be too subtle.
f1a6b77 to
77e659f
Compare
eed3si9n
left a comment
There was a problem hiding this comment.
Pending the discussion #3865 (comment)
77e659f to
168f2b9
Compare
.. and appendWithoutSession.
168f2b9 to
0885233
Compare
|
Should this be merged into 1.1.x or 1.x. Hmm. (My eternal bug fix vs improvement debate..) |
|
For sbt, the point of a hotfix release is to improve the quality (often by fixing bugs) without introducing new breakages brought in by new features. Since introducing new method should not break existing code paths, this should a safe patch. |
|
certainly seems appropriate for 1.1.1 to me (and besides, it would be sad if the Scala build had to wait for 1.2) |
Adds
Extracted#appendWithSessionas a better alternative toExtracted#appendbecause it preserves session settings.appendin conjunction with+and finding thatappenddiscarded thescalaVersionthat+set in the session.appendwas discarding the settings set with theapplycommand.Extracted#appendand usedLoad, which as of sbt 1 is now internal API.