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

Add gis_admin_deprecations fixer #484

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

fitoria
Copy link

@fitoria fitoria commented Aug 19, 2024

Allows replacement of OSMGeoAdmin and GeoModelAdmin with GISModelAdmin from the same module, those classe were removed on Django 5.0: https://docs.djangoproject.com/en/5.0/releases/5.0/#django-contrib-gis

@adamchainz
Copy link
Owner

Oh great. I haven't got time for a full review right now, but at a glance it’s looking good. Can you add a changelog note?

Little note for the future - please don't make PRs from the main branch of your fork, but create a branch. It makes it easier for maintainers to push edits back and be sure that things don't break.

@fitoria
Copy link
Author

fitoria commented Aug 20, 2024

Ready!

@adamchainz adamchainz changed the title Adds fixer: gis_admin_deprecations for Django 5.0 Add gis_admin_deprecations fixer Aug 26, 2024
@adamchainz
Copy link
Owner

This would be a Django 4.0 fixer, because that's when the classes were deprecated. I have moved it there.

I looked at the code as of Django 4.0. It looks like we can't blindly rewrite model admin classes to use GISModelAdmin in the case that they define any of the 29 attributes provided by the old classes: https://github.com/django/django/blob/4555aa0a489cb9dcf764edf12339097cdfa5ff84/django/contrib/gis/admin/options.py#L43-L71 . Doing so could break functionality.

Those attributes could also come from multiple inheritance, which should also mean that a safe automated fix is impossible.

So I think we should add two restrictions:

  1. Only rewrite model admin classes that inherit from exactly one class, with no keyword arguments (i.e. not class F(OSMGeoAdmin, something=1):, just in case)
  2. Ignore any classes that have any the 29 attributes defined in their class bodies.

Do you think you could update to add these, and appropriate tests?

@adamchainz adamchainz self-requested a review August 26, 2024 13:45
@fitoria
Copy link
Author

fitoria commented Aug 26, 2024

Do you think you could update to add these, and appropriate tests?

Sure, I'm going to take care of it, thanks for the review.

@adamchainz
Copy link
Owner

Hey, I am just gearing up for a release here, so I took a look at your PR. It needs quite a bit of work to limit the fixer to class definitions and add the above restrictions. I pushed two new, failing tests that represent the restrictions, and will leave the PR like this for now. Let me know if you find time to work on this again (no pressure!).

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