Fix is confirmed before bug #858
Merged
Conversation
This file contains 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
…ry conditions, remove obsolete comment
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR is mostly a cherry-pick of a commit from the spend_kit branch (see commit message).
I will break up my rationale for this PR into two parts.
How I Fixed The Bug
The bug was caused by a tiny smidge of wetness, and copy pasta, coupled with the false belief that the doc tests were protecting us.
The fix is both correct, and DRYer because it uses composition of relevant functionality rather re-expression of the same thing.
(I also tightened boundary conditions and added assertions to the doc-tests.)
Why I Propose This Way of Organizing PRs
If this PR is accepted into
devit will have some nice properties:(1) It will have made the length of the feature branch (as measured in diff from dev) smaller. I assume this is obviously good.
(2) It will have decreased the length of distance from the tip of the feature, to the tip of any other feature.
Here's the thought experiment:
Oscar is working on some other feature (some sync thing) at the same time. This PR lands (maybe Oscar reviews it).
Now what?
There's this chunk of code (on dev) that doesn't really do anything (on dev), but it's probably necessary for spend_kit (its "origin feature") and some_sync_thing is aware of it. This means that (a) some_sync_thing won't accidentally put something else there. Maybe some_sync_thing will actually use it. I dunno'.. but at the end of the day all the
developing features need to work together. By sucking pieces of them intodev, when it is convenient to do so, we ensure that features being developed againstdevare aware of each other earlier.