-
-
Notifications
You must be signed in to change notification settings - Fork 63
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
Rewrite admin.site.register()
to @admin.register()
#182
Comments
This could be an interesting fixer to have, but it would also be more of a coding style choice. The "direct register" approach is still used in Django's examples, and still needed for cases where model classes don't map to admin classes one-to-one. Also, wouldn't the caveat also extend to other references to the class within the body, e.g. in other |
Thanks for the quick answer! I agree it's more of a coding style thing (although I do think it makes the navigation easier).
I had this point in mind but I think we can deal with it, if a custom admin classe maps to several models, it's either all happening in one file and we can track the number of usage and skip rewrite if it's more than one OR the custom admin class is imported in another file and in that case we skip the rewrite anyway. Will need a few test cases but I think it's manageable.
It has to do with the class creation and the way the decorator works, initially was flagged for the |
One edge case is that an admin can be registered against multiple models: +@admin.register(MyModel1, MyModel2)
class MyCustomAdmin:
pass
-admin.site.register(MyModel1, MyCustomAdmin)
-admin.site.register(MyModel2, MyCustomAdmin) Probably not used very much, but that might be a small complication that I think is worth mentioning. |
Given the predicted complexity of this fixer, the caveat about It could be easier to use a flake8 plugin that tells developers to to prefer the decorator syntax. |
This is an attempt to change your mind on this. I've added a draft PR and the implementation is actually not that complex. While it's not dealing with a deprecation, it still aims at using newer django feature (like the httpRequest rewritter which is quite cool). I was thinking of maybe having a About the flake8 option, I find it's adding a lot less friction if a formatter can do the same. Anyhow, thanks for the package, I'm enjoying a lot working on it! |
Okay, considering that there's a pretty solid implementation, it's definitely worth discussing. Glad you had fun working on this. The PR looks pretty good from a first read, and you've covered many bases. One thing springs to mind - we didn't discuss custom admin sites yet... e.g. custom_site = AdminSite() # or 'from ... import custom_site'
+admin.register(MyModel, site=custom_site)
class MyModelAdmin(...):
...
- custom_site.register(MyModel, MyModelAdmin) I've seen custom sites used a few times but I am less certain they are often used. I think we could leave them, at least for now, but wanted to discuss it first. Detecting a custom site class is a little hard from the AST, especially since it could be imported from elsewhere. We could use some heuristics like "name of site variable ends in 'site'" and "name of second argument to |
Thanks for the kind words! Custom admin sites crossed my mind too when I was looking into the decorator internals but I'm not really familiar with them (and how people usually use them) so I was not really confident trying to support them. The heuristics you mention sound reasonable but again I'm not familiar enough with custom site to confirm. Maybe keep that for a later PR ? About another thing we didn't speak about, I found another use case maybe worth mentioning: +admin.register(MyModel)
class MyModelAdmin(...):
- save_as=...
+ save_as=True
...
-admin.site.register(MyModel, MyModelAdmin, save_as=True) With However, it looks like a really rare use case and I'm not sure it's worth spending time on that for now. oh and what are your thoughts about maybe having a flag for the non directly deprecation-related rewrites ? |
Okay the main fixer was added in #189 , thank you! 👏👏👏 I'm not sure about supporting the extra arguments. It's not 100% safe to modify the main class, since Django creates a subclass and so the attribute "isn't there" in the main class. There could be several registered classes spread between files and rewriting this case would affect them all. I've opened #190 with the further cases that I think are safe enough to support. Let's continue the discussion there. |
Just tested this against a client project, it rewrote a bunch of |
Description
Description
We could rewrite
admin.site.register()
to the more elegant@admin.register()
syntax :This syntax was actually added a while ago (Django1.7).
I usually find it a bit easier to use since the Model and associated Custom Admin definition stay really close.
Potential Issue
There is a known issue when using a python 2 style super call in the
__init__
method. The fixer should not rewrite in this case.Let me know if this is something you'll be interested in, I'll be happy to submit a PR.
Cheers
The text was updated successfully, but these errors were encountered: