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.

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

DX: Remove db-diff dependency #288

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Conversation

pfouque
Copy link

@pfouque pfouque commented Jan 9, 2024

Used only for testing, mainly using assertNoDiff only.
It creates some unnecessary back and forth between these 2 repository.

This PR tries to get rid of this dependency so that django-cities-light is more independent

Comment on lines +9 to +10
def ready(self):
register_serializer('sorted_json', 'cities_light.serializers.json')
Copy link
Author

@pfouque pfouque Jan 9, 2024

Choose a reason for hiding this comment

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

Register an additional serializer where the dbdiff package was overriding the default json serializer.

Comment on lines +32 to +46
{
"fields": {
"alternate_names": "Юргинский район",
"country": [2017370],
"display_name": "Yurginskiy Rayon, Russia",
"geoname_code": "1485714",
"geoname_id": 1485714,
"name": "Yurginskiy Rayon",
"name_ascii": "Yurginskiy Rayon",
"region": [1503900],
"slug": "yurginskiy-rayon"
},
"model": "cities_light.subregion",
"pk": 1
},
Copy link
Author

Choose a reason for hiding this comment

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

I noticed that this fixture wasn't up to date :/

@pfouque pfouque mentioned this pull request Jan 9, 2024
@marianoeramirez
Copy link
Collaborator

Do you ser necessary continue with the Remove db-diff?

Because know I have access to that repo and could keep updated

@marianoeramirez
Copy link
Collaborator

@pfouque ?

"makemigrations",
"cities_light",
"--dry-run",
"--check",
Copy link
Author

Choose a reason for hiding this comment

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

Call the real makemigrations command instead of the MigrationLoader.
As it's kind of a public interface it should be more stable (it's also shorter and easier to test out of the tests)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I like it, and some other modifications.

Copy link
Author

Choose a reason for hiding this comment

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

Nice, I will try to move forward and get a stable state.
That would make this package more independent and easier to maintain

@pfouque
Copy link
Author

pfouque commented Jan 15, 2024

Do you ser necessary continue with the Remove db-diff?

Because know I have access to that repo and could keep updated

Up to you, I started this when it wasn't clear if db-diff would be fixed, and I discovered it was used mainly for a single assert. (If it was my project I would probably incorporate it directly, but up to you)
In the meantime, feel free to cherry-pick whatever you want from this PR

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.

2 participants