8000 Fixed #32132 -- Fixed column types in m2m intermediary tables for Positive(Big/Small)IntegerFields. by David-Wobrock · Pull Request #13592 · django/django · GitHub
[go: up one dir, main page]
Skip to content

Fixed #32132 -- Fixed column types in m2m intermediary tables for Positive(Big/Small)IntegerFields. #13592

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.

Sign up for GitHub

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

Jump to bottom
Merged
8000
merged 2 commits into from
Nov 2, 2020
Merged

Fixed #32132 -- Fixed column types in m2m intermediary tables for Positive(Big/Small)IntegerFields. #13592

merged 2 commits into from
Nov 2, 2020

Conversation

David-Wobrock
Copy link
Member

Ticket: https://code.djangoproject.com/ticket/32132

Hello 👋

I basically added a type switching when getting the related field type for the PositiveBigIntegerField and PositiveSmallIntegerField, as they are defined/used by the BigIntegerField and SmallIntegerField, instead of hard-coding the IntegerField :)

I'm not 100% of the side-effects linked to inheriting from BigIntegerField and SmallIntegerField. But I would expect it to be rather limited and in the end more beneficial than harmful.

The PR is also missing unit tests. Please let me know if I should add some (and if yes, where those are located, I had trouble finding this logic in the tests)

charettes
charettes reviewed Oct 23, 2020
View reviewed changes
@smithdc1
Copy link
Member

Yes please for tests. Maybe here?

https://www.github.com/django/django/tree/master/tests%2Fmodel_fields%2Ftest_integerfield.py

@David-Wobrock
Copy link
Member Author

Hi @charettes and @smithdc1
Thanks for the help! I improved the lookup of the base IntegerField class a bit and added unit tests for the rel_db_type of the different IntegerField classes

charettes
charettes reviewed Oct 25, 2020
View reviewed changes
Copy link
Member
@charettes charettes left a comment

Choose a reason for hiding this comment

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

Thanks for the tests @David-Wobrock.

I think we should base tests expected values of connection.features.related_fields_match_type instead of the raw DDL otherwise it will be hard for custom backends running the Django test suite to opt-in into these tests.

charettes
charettes reviewed Oct 25, 2020
View reviewed changes
@David-Wobrock David-Wobrock force-pushed the ticket-32132 branch 3 times, most recently from 46848db to e4bb09f Compare October 25, 2020 20:26
@David-Wobrock
Copy link
Member Author

I adapted the tests, but still didn't switch on related_fields_match_type. Mostly because the AutoField, which also inherit from the same TestCase, have a different rel_db_type logic, which doesn't involve the related_fields_match_type boolean

@felixxm felixxm changed the title Fixed #32132 -- Corrected related field type of PositiveBigIntegerField Fixed #32132 -- Fixed column types in m2m intermediary tables for Positive(Big/Small)IntegerFields. Nov 2, 2020
@felixxm
Copy link
Member
felixxm commented Nov 2, 2020

@David-Wobrock Thanks 👍 I pushed small edits and moved extra tests to a separate commit.

felixxm
felixxm reviewed Nov 2, 2020
View reviewed changes
@felixxm felixxm merged commit cfc7cd6 into django:master Nov 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@charettes charettes charettes left review comments

@felixxm felixxm felixxm left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@David-Wobrock @smithdc1 @felixxm @charettes
0