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.

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

Fixed #31416 -- Made autodetector find dependencies for MTI model creation on base fields removal. #12754

Merged
merged 1 commit into from May 26, 2020
Merged

Fixed #31416 -- Made autodetector find dependencies for MTI model creation on base fields removal. #12754

merged 1 commit into from May 26, 2020

Conversation

nanlliu
Copy link
Contributor

@nanlliu nanlliu commented Apr 19, 2020

adamchainz
adamchainz reviewed Apr 20, 2020
View reviewed changes
Copy link
Sponsor Member

@adamchainz adamchainz left a comment

Choose a reason for hiding this comment

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

Overall looks like it will work.

Simon wrote on the ticket about doing this with check_dependency - was this not feasible?

django/db/migrations/autodetector.py Outdated Show resolved Hide resolved
django/db/migrations/autodetector.py Outdated Show resolved Hide resolved
@nanlliu
Copy link
Contributor Author

nanlliu commented Apr 20, 2020 •
edited

Overall looks like it will work.

Simon wrote on the ticket about doing this with check_dependency - was this not feasible?

It wasn't necessary from my perspective, and also someone else also thought it was unnecessary. And i did find out that adding the dependencies would get the job done. And Simon stated it may be related to check_dependency, so i guess he wasn't 100% sure as well?

@nanlliu
Copy link
Contributor Author

nanlliu commented Apr 20, 2020 •
edited

Not sure why some checks failed after removing some redundant parts of the code. Is it configuration errors?

@adamchainz
Copy link
Sponsor Member

If you read the logs, you'll see the build failed because Jenkins was trying to test a commit it couldn't get hold of: https://djangoci.com/job/pull-requests-bionic/database=postgis,label=bionic-pr,python=python3.8/5417/console

For Django commit style, you should squash them all into one, titled after the PR: "Fixed #31416 -- FieldError when migrating field to new model subclass." This will cause a rebuild and that should pass.

@nanlliu
Copy link
Contributor Author

nanlliu commented Apr 20, 2020

now all checks have passed! is there anything that i may need to check? much appreciated for the help!

@nanlliu nanlliu requested a review from adamchainz April 21, 2020 00:27
@adamchainz
Copy link
Sponsor Member

Hi @nanlliu

I appreciate your eagerness, but please be patient. We're all doing Django work in our spare time. It can sometimes take me weeks to find the time to come back to a review.

I think this PR is in good shape, but I suspect @charettes will want to review as he knows more about the migrations framework and commented on the ticket, and then a fellow will do a pass before merging it. They will get to it in their own time.

Thanks,

Adam

charettes
charettes approved these changes May 23, 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.

Changes look fine to me, looks like this is a simpler approach than what I had in mind.

django/db/migrations/autodetector.py Outdated Show resolved Hide resolved
django/db/migrations/autodetector.py Show resolved Hide resolved
tests/migrations/test_autodetector.py Outdated Show resolved Hide resolved
@nanlliu nanlliu force-pushed the dev/31416 branch 2 times, most recently from 93a24df to 5df9bb2 Compare May 24, 2020 01:00
@felixxm felixxm self-assigned this May 25, 2020
@felixxm felixxm removed the request for review from adamchainz May 25, 2020 10:41
@felixxm felixxm changed the title Fixed #31416 -- FieldError when migrating field to new model subclass. Fixed #31416 -- Made autodetector find dependencies for MTI model creation on base fields removal. May 26, 2020
@felixxm
Copy link
Member

felixxm commented May 26, 2020

@nanlliu Thanks 👍 Welcome aboard ⛵

I pushed small edits.

@nanlliu @felixxm
Fixed #31416 -- Made autodetector find dependencies for MTI model cre…
33c3657 
…ation on base fields removal.

Removing a base field must take place before adding a new inherited
model that has a field with the same name.
@felixxm felixxm merged commit 33c3657 into django:master May 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adamchainz adamchainz adamchainz left review comments

@charettes charettes charettes approved these changes

Assignees

@felixxm felixxm

Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@nanlliu @adamchainz @felixxm @charettes