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

Show what module/function is missing when raised. #2871

Closed
wants to merge 1 commit into from

Conversation

cwood
Copy link
Contributor

@cwood cwood commented Jul 1, 2014

Related to comment on 98949e3

@timgraham
Copy link
Member

Could you add a test?

@cwood
Copy link
Contributor Author

cwood commented Jul 2, 2014

Yea I can make a test for it. Wasn't sure it needed it since it was just adding the missing variables.

@timgraham
Copy link
Member

Thanks I created a ticket for the issue so you can refer to it in the commit message.

See also our patch review checklist.

@cwood
Copy link
Contributor Author

cwood commented Jul 2, 2014

Okay cool. Sorry for making you do all this work I should have done it. I just wasnt sure what it warrented.

return "somewhere dynamic"
thing = models.FileField(upload_to=upload_to)

with self.assertRaisesRegexp(ValueError,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to use six.assertRaisesRegex for Python 3 compatibility

@timgraham
Copy link
Member

Could you squash commits and follow the commit message guidelines as noted in the checklist I linked above? Note since this PR is against stable/1.7.x the message should be prefixed with [1.7.x]. For future reference, it's a tiny bit easier if you submit the PR against master and then let the committer backport from there, but it's not worth creating a new PR at this point. Thanks!

@cwood
Copy link
Contributor Author

cwood commented Jul 2, 2014

I squashed them down and updated the message. Ill make sure to do that next time.

@timgraham
Copy link
Member

Oops, the test didn't actaully work after switching to six, but I fixed it and merged in f5740af. Thanks Colin.

@timgraham timgraham closed this Jul 2, 2014
@cwood
Copy link
Contributor Author

cwood commented Jul 2, 2014

Thanks Tim! Sorry for all the issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants