3YOURMIND / django-add-default-value

This django Migration Operation can be used to transfer a Fields default value to the database scheme.
Apache License 2.0
138 stars 19 forks source link

Quote Bug Fix: set quotes back to 'default' DB #12

Closed David-Wobrock closed 5 years ago

David-Wobrock commented 5 years ago

When different databases are set up, we might be using MSSql or MySQL quotes for a Postgresql DB. See description in issue #11

Fixes #11

Ping @mes3yd since you seem to manage this repo. I would be very grateful if you could review, test, merge and publish to PyPi this bug fix ASAP. I'm working on a large refactoring of the migration linter (https://github.com/3YOURMIND/django-migration-linter) and this bug fix here would be appreciated to avoid that the tests there fail.

I'll try to do another PR anytime soon to refactor and try simplifying the code. In the current state, it is very complex complicated for such a simple task :thinking:

ghost commented 5 years ago

I'll clean it up a bit and get rid of default_vendor as that stimulates these kinds of bugs. Also want to address: https://code.djangoproject.com/ticket/30371#comment:8, so I'll get a fix out today or tomorrow.

David-Wobrock commented 5 years ago

Okay, great! Thanks a lot

ghost commented 5 years ago

I'm taking this fix for now, since my refactorment is a lot more involved.

Also, it was a simple extension but is no longer because:

Right now it's a little more complex as it can be, since I've abstracted 3 quote values that are identical accross all backends and I've scheduled it for removal if we don't add more engines that deviate from this. But in principle, the code for database_forwards is very slim and if you start reading there, it's not as complex as you might think. In my refactorment I'm pulling out a few more things to make it only call functions/methods in the right order and pass logic that requires knowledge of the vendor / engine to others.