search

coderholic / django-cities

Countries and cities of the world for Django projects
MIT License
920 stars 374 forks source link

support django 2 #185

Closed mehdipourfar closed 6 years ago

mehdipourfar commented 6 years ago

In Django 2, on_delete argument for ForeignKey is required. I have created a SET_NULL_OR_CASCADE function which calls SET_NULL when null argument of the field is True, and otherwise calls CASCADE functions.

GeoManager is also deleted from gis models. Django docs. Since app code doesn't depend on it, I check if django version is below 2, then use GeoManager, else use default Manager.

madisvain commented 6 years ago

This works just fine and seems to fix things for 2.0

blag commented 6 years ago

Dunno why the builds broke but I think that is unrelated to this change.

Thank you for putting this together for me, I don't have as much free time to maintain this package as I used to.

However, I do not like modifying the migration files directly - that alone means this gets an automatic NAK from me, as existing users will never run those migrations if they've already run migration 0001_initial before this change. This is explained below.

I have pulled these changes into #188 and will be fixing that there. Please see that PR for updates. Closing.

mehdipourfar commented 6 years ago

Changing migrations files is unavoidable because on_delete is no longer an optional argument for ForeignKeys in Django 2.

mehdipourfar commented 6 years ago

And also notice that it does't matter if existing users couldn't run changes. Changes are supposed to maintain database states as it was before.

blag commented 6 years ago

Changing migrations files is unavoidable because on_delete is no longer an optional argument for ForeignKeys in Django 2.

Oh, derp, excellent point.

And also notice that it does't matter if existing users couldn't run changes. Changes are supposed to maintain database states as it was before.

So if I understand on_delete correctly, you have preserved the behavior of on_delete from previous versions of Django? So the previous behavior of Django matches your SET_NULL_OR_CASCADE function?

mehdipourfar commented 6 years ago

Not exactly. But it doesn't matter. SET_NULL_OR_CASCADE checks if the field accepts null or not. If it accepts, it uses SET_NULL, otherwise it uses CASCADE. The main difference in behavior is for City's ForeignKey to country. In old version, null was true and on_delete was CASCADE by default. Now on_delete is SET_NULL. You should make another migration for this. I've forgotten to do it.

blag commented 6 years ago

@mehdipourfar Can you review #188 for me?

I'm also unclear on what type of migration I should add. Can you explain more please? Thanks!

mehdipourfar commented 6 years ago

just run a makemigrations.