Closed
Conversation
ghost
mentioned this pull request
Collaborator
|
I second @thedrow's comment on #2639 that there should be a test case here. It needs to demonstrate the issue that's being fixed. It should fail without the patch and pass with it. Such tests bring to light the expected behaviour — they help to decide if that's the behaviour we want. (They also catch regressions down the line.) My immediate thought though is that this looks similar to #2637 so we need to make sure we're not making two related changes where one is required. (That's another way the test would help.) |
ghost
mentioned this pull request
Collaborator
|
Closing as per comment on #2638 |
This was referenced Mar 9, 2017
This was referenced Oct 16, 2017
This was referenced Nov 6, 2017
This was referenced Nov 14, 2017
This was referenced Dec 10, 2017
This was referenced Dec 20, 2017
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Updated patch addressing @thedrow 's comments.