Skip to content
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

Fix no pk issue #2638 #2641

Closed
wants to merge 1 commit into from
Closed

Fix no pk issue #2638 #2641

wants to merge 1 commit into from

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

carltongibson commented Mar 5, 2015

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

carltongibson commented Mar 8, 2015

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.

None yet

2 participants