search

taobojlen / django-zeal

Detect N+1s in your Django app.
MIT License
90 stars 1 forks source link

Exception message and allow list improvements #25

Open serl opened 2 months ago

serl commented 2 months ago

As I installed zeal in a big project, it founds lots of little violations here and there. Fixing everything would make a confusing PR, so I thought about starting by installing the tool, ignoring all the existing violations, the fix them one by one. At least the list should not grow in the meantime!

So I had to add ~30 fields to the ZEAL_ALLOWLIST, and these little things would have helped me:

  1. Having the app name in the error message. Of course I know that User is auth.User, but I suspect zeal could know as well and it would have been easier to copy/paste, so a better message would be "N+1 detected on auth.User.my_field with calls:"
  2. Accepting the same format in the allow list. {"model": "auth.User", "field": "my_field"} is very clear, but "auth.User.my_field" is also clear (I mean otherwise the exception would have said something different). What about accepting both?
  3. Of course a couple of times I put the wrong app name, and the tests would fail in the exact same manner as before. What about adding a startup check to ensure that the fields/models actually exist? This would also ensure that when a model is deleted (we delete a lot), the cleanup is complete.

If you agree on all or any, and if you wish to receive PRs, I'd like to participate. Just let me know! 🙂