Skip to content

Conversation

@ghost
Copy link

@ghost ghost commented Mar 5, 2015

Updated patch addressing @thedrow 's comments.

@ghost ghost changed the title Fix no pk issue #2639 Fix no pk issue #2638 Mar 5, 2015
@carltongibson
Copy link
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.)

@carltongibson
Copy link
Collaborator

Closing as per comment on #2638

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants