Skip to content

Conversation

jbzdak
Copy link
Contributor

@jbzdak jbzdak commented Nov 6, 2015

Inspectdb now handles renamed fields in unique_together.

It was tested on sqlite and postgresql (I don't know whether I'm supposed to check it on other all databases).

@jbzdak jbzdak changed the title Fixes #25274 --- inspectdb handles renamed fields in unique_togeher. Fixed #25274 --- inspectdb handles renamed fields in unique_togeher. Nov 6, 2015
@jbzdak jbzdak force-pushed the ticket_251274 branch 2 times, most recently from 40e99d6 to e92d198 Compare November 6, 2015 16:52
Inspectdb now handles renamed fields in unique_together.
Copy link
Member

Choose a reason for hiding this comment

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

Add a trailing comma.

@jbzdak
Copy link
Contributor Author

jbzdak commented Nov 10, 2015

I have added suggested changes, two questions:

  1. Should I squash those two commits
  2. After browsing code for second time I'm wondering if tests could be done better. Here is the issue: Tests implicitly test for the order of unique_togetgher entries so if two columns were present in unique_together in reverse order ("foo", "bar") instead of ("bar", "foo") this would be unittest error, while at the database level these two constraints are equivalent (as far as I know). I could amend tests to fix this behaviour, but it sould decrease test clarity. Is this worthwhile?

@timgraham timgraham changed the title Fixed #25274 --- inspectdb handles renamed fields in unique_togeher. Fixed #25274 --- Made inspectdb handle renamed fields in unique_together. Nov 25, 2015
@timgraham
Copy link
Member

Splitting up the test into a class didn't seem to add clarity, so I moved it back into a method. I didn't look closely about the issue with field name ordering, but I haven't seen the test failure non-deterministically so I think we're okay for now. Merged in 2cb50f9, thanks!

@timgraham timgraham closed this Nov 25, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants