Skip to content
This repository has been archived by the owner on Feb 22, 2023. It is now read-only.

Remove get_api_exception and replace with built in DRF exceptions #843

Closed
sarayourfriend opened this issue Aug 2, 2022 · 5 comments · Fixed by #976
Closed

Remove get_api_exception and replace with built in DRF exceptions #843

sarayourfriend opened this issue Aug 2, 2022 · 5 comments · Fixed by #976
Assignees
Labels
💻 aspect: code Concerns the software code in the repository 🧰 goal: internal improvement Improvement that benefits maintainers, not users good first issue New-contributor friendly help wanted Open to participation from the community 🟩 priority: low Low priority and doesn't need to be rushed

Comments

@sarayourfriend
Copy link
Contributor

sarayourfriend commented Aug 2, 2022

Description

The bespoke get_api_exception function recreates built in DRF functionality and makes searching for particular exceptions more difficult. Instead of searching for raise NotFound or raise <built-in-DRF-exception>, the typical way to find code that explicitly raises exceptions, you'd have to know about the get_api_exception function and search for usages with the specific HTTP status code (which has to be hand written by the user each time, meaning they have to remember what the status code is).

It also obscures stack traces because all errors created by get_api_exception have the same name (only solvable by further meta programming using the type constructor instead of a closed class): https://sentry.io/share/issue/abf49297eb2749e889eff15e4d37e151/

In short: we can eliminate code by removing this, be more in line with default DRF functionality and the community.

  1. Replace all usages of get_api_exception function with the appropriate built-in DRF exception from rest_framework.exceptions. For any that are following the try: queryset.get(); except Model.DoesNotExist: ... pattern, replace it with get_object_or_404.
  2. Remove the get_api_exception function
@sarayourfriend sarayourfriend added good first issue New-contributor friendly help wanted Open to participation from the community 🟩 priority: low Low priority and doesn't need to be rushed 💻 aspect: code Concerns the software code in the repository 🧰 goal: internal improvement Improvement that benefits maintainers, not users labels Aug 2, 2022
@AbhiYHub
Copy link
Contributor

AbhiYHub commented Aug 2, 2022

Hello @sarayourfriend, I am new to open source and I would like to work on this.

@sarayourfriend
Copy link
Contributor Author

Hi @AbhiYHub! That's great, thank you for your willingness to contribute 🙂 I've assigned the issue to you. If you have any questions here or run into issues during implementation, please feel free to ping me or @WordPress/openverse-maintainers for help.

@abhi2296
Copy link

Hello @sarayourfriend !! I am looking to start my journey in Open Source projects. Could you please assign this issue to me?

@dhruvkb dhruvkb assigned abhi2296 and unassigned AbhiYHub Sep 10, 2022
@AbhiYHub
Copy link
Contributor

AbhiYHub commented Oct 7, 2022

Hello @sarayourfriend and @dhruvkb, Apologies for the delay. I have solved the issue and also ran the test cases. All the test cases have passed successfully. I would request if you can reassign me the task so that I could submit the solution.

@dhruvkb
Copy link
Member

dhruvkb commented Oct 7, 2022

@AbhiYHub please push your solution as a PR. Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
💻 aspect: code Concerns the software code in the repository 🧰 goal: internal improvement Improvement that benefits maintainers, not users good first issue New-contributor friendly help wanted Open to participation from the community 🟩 priority: low Low priority and doesn't need to be rushed
Projects
None yet
4 participants