-
Notifications
You must be signed in to change notification settings - Fork 215
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
Replace function-local dict media type -> model relationship configurations with static module dicts or an otherwise shared approach #4553
Comments
Adding a note here that the approach described in suggested improvement point 1 would be similar to this test fixture. openverse/api/test/fixtures/media_type_config.py Lines 72 to 111 in 73c0ad5
I would also favour the second approach, but that's because I expect a number of circular dependency issues to come up using the first approach. |
I'm unassigning myself this for now and closing #4599. I need to focus on some other work at the moment and do not have time to rebase and get that PR reviewable. There is some good feedback and discussion in the PR, and @krysal shared a preference for the cached property approach, though I personally don't see how it's that much less meta-programming than the metaclass approach, and it does not completely solve the issue of fiddly configuration required on all of these support models, namely the related fields, which are arguably one of the most important things to get right here, and the most annoying to deal with when they aren't. In fact, if just the related names were unified, at least you can get the model from most of these relations on the class. I think the meta-class approach works well, and don't fully understand the trepidation towards hooking into Django in this way, but do understand that not everyone is comfortable with this kind of approach and would prefer verbose manual configuration of these generic relations rather than programatically handling it as I've done in the PR. Regardless, I'm happy to review and give help if someone else takes this up and solves it. Hopefully it's clear from the PR how much of an improvement it is in all the code working on these relationships. |
Current Situation
See this thread for context: #4544 (comment)
This pattern exists widely in our code, and does not need to, and has the following current and potential problems:
Suggested Improvement
Either:
To me the latter is preferred because it creates a much cleaner and more consistent code, without the need to manually maintain dicts of these relationships at all.
Benefit
Avoid the anti-pattern qualities of the function-local inline dict approach.
The text was updated successfully, but these errors were encountered: