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

Remove unnecessary boilerplate implementations of get_media_type #1385

Closed
1 task
sarayourfriend opened this issue Oct 24, 2022 · 3 comments · Fixed by #4061
Closed
1 task

Remove unnecessary boilerplate implementations of get_media_type #1385

sarayourfriend opened this issue Oct 24, 2022 · 3 comments · Fixed by #4061
Assignees
Labels
💻 aspect: code Concerns the software code in the repository ✨ goal: improvement Improvement to an existing user-facing feature good first issue New-contributor friendly 🟩 priority: low Low priority and doesn't need to be rushed 🧱 stack: catalog Related to the catalog and Airflow DAGs
Projects

Comments

@sarayourfriend
Copy link
Contributor

Current Situation

In WordPress/openverse-catalog#821 we introduced a default implementation for ProviderDataIngester::get_media_type that removes the need for a large number of boilerplate-y implementations of the function in existing provider classes.

Suggested Improvement

We can remove those unnecessary lines of code now that the default implementation covers them. This only applies to providers that handle a single media type.

Benefit

Less code 🙂

Implementation

  • 🙋 I would be interested in implementing this feature.
@sarayourfriend sarayourfriend added good first issue New-contributor friendly 🟩 priority: low Low priority and doesn't need to be rushed ✨ goal: improvement Improvement to an existing user-facing feature 💻 aspect: code Concerns the software code in the repository labels Oct 24, 2022
@obulat obulat added the 🧱 stack: catalog Related to the catalog and Airflow DAGs label Feb 23, 2023
@openverse-bot openverse-bot added this to Backlog in Openverse Apr 17, 2023
@obulat obulat transferred this issue from WordPress/openverse-catalog Apr 17, 2023
@ngken0995
Copy link
Collaborator

@sarayourfriend Does this require going to each provider in catalog/dags/providers/provider_api_script and remove the get_media_type function where providers handle a single media type?

@sarayourfriend
Copy link
Contributor Author

sarayourfriend commented Nov 7, 2023

That's the right idea. I can't remember exactly now, but there might be some cases where it's still necessary to keep the provider-specific implementation, but that some parts of the method could be replaced with a call to super::get_media_type. In other words, you're on the right track, just make sure that you only remove the parts that the base class implements but keep any provider-specific aspects of the implementation.

@zaharoian
Copy link
Contributor

Interested in this issue!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💻 aspect: code Concerns the software code in the repository ✨ goal: improvement Improvement to an existing user-facing feature good first issue New-contributor friendly 🟩 priority: low Low priority and doesn't need to be rushed 🧱 stack: catalog Related to the catalog and Airflow DAGs
Projects
Archived in project
Openverse
  
Backlog
4 participants