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

Invalid/Unused target_class should raise warning #114

Closed
jayvdb opened this issue Jul 8, 2020 · 8 comments
Closed

Invalid/Unused target_class should raise warning #114

jayvdb opened this issue Jul 8, 2020 · 8 comments

Comments

@jayvdb
Copy link
Contributor

jayvdb commented Jul 8, 2020

If a fixer/extension mentions a target_class which either doesnt exist, or wasnt seen at all during urlconf processing, it would be useful to have a warning.

This can occur due to typos, or can occur because a third-party app renames stuff internally - they usually assume that their view class names are not part of their public API unless they document them as extendable.

@tfranzel
Copy link
Owner

tfranzel commented Jul 8, 2020

the flexibility of extensions does come at a price. the built-in extensions would emit senseless warning if a lib was not installed. we could theoretically add an option for missing classes to the extensions. on the other hand, i would prefer to not further complicate the extensions for the user. it's a trade-off and i'm not sure.

@jayvdb
Copy link
Contributor Author

jayvdb commented Jul 8, 2020

What about detecting that the lib is in INSTALLED_APPS, and only warn for view names within those libs?

@jayvdb
Copy link
Contributor Author

jayvdb commented Jul 8, 2020

Also a nice implementation of local vs non-local at kalekseev/django-extra-checks#2 ; if the extension exists in local code, that is highly likely to be important, whereas any extension appearing in drf_spectacular code would be non-local in that algorithm.

@tfranzel
Copy link
Owner

tfranzel commented Jul 8, 2020

this should guard against upstream refactorings and most typos. it is based on installed_apps. if an app is not officially installed or the app name has a typo there is no warning. it has the benefit of only being a small internal change. i think that should service most use cases.

@jayvdb
Copy link
Contributor Author

jayvdb commented Jul 8, 2020

Looks like it suffers the same problem as kalekseev/django-extra-checks#6

@tfranzel
Copy link
Owner

tfranzel commented Jul 8, 2020

does it? in case the app -- from which the class originates -- is not listed in installed_apps, the warning simply does not happen as i explained above. the outcome is still the same. it is a tradeoff. beyond that, the complexity and required heuristic of capturing all cases would outweigh the benefits.

tfranzel added a commit that referenced this issue Jul 11, 2020
@tfranzel
Copy link
Owner

@jayvdb i had another look here and now i get what you meant.

https://docs.djangoproject.com/en/3.0/ref/settings/#installed-apps

Your code should never access INSTALLED_APPS directly. Use django.apps.apps instead.

i tested it with 'oauth2_provider.apps.DOTConfig' which we have available and it looks good to me. it gets correctly resolved to target_class.startswith('oauth2_provider.') this is resolved as far as i'm concerned.

@jayvdb
Copy link
Contributor Author

jayvdb commented Jul 11, 2020

Lovely!

@jayvdb jayvdb closed this as completed Jul 11, 2020
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

No branches or pull requests

2 participants