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.

Sign up for GitHub

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

Jump to bottom

Configure isort to include source paths #337

Closed
AetherUnbound opened this issue Nov 28, 2022 · 4 comments · Fixed by WordPress/openverse-catalog#1054
Closed

Configure isort to include source paths #337

AetherUnbound opened this issue Nov 28, 2022 · 4 comments · Fixed by WordPress/openverse-catalog#1054
Assignees
@raiyaj
Labels
🤖 aspect: dx Concerns developers' experience with the codebase 🧰 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 🧱 stack: api Related to the Django API 🏁 status: ready for work Ready for work

Comments

@AetherUnbound
Copy link
Collaborator

Description

It would be nice to have our isort configurations in our .pre-commit-config.yaml file set up to recognize the repo-specific source paths: https://pycqa.github.io/isort/docs/configuration/options.html#src-paths

This would separate internal imports (1st party) from our other dependencies (3rd party) in the import statements. Presently the two are conflated and grouped in the same block:

https://github.com/WordPress/openverse-catalog/blob/dad3cb49618f38aadb3ae7772f98ca77180bf35c/openverse_catalog/dags/database/image_expiration_workflow.py#L8-L11

I believe this is reliant on #334 since we would also need use the repo-specific folders (i.e. openverse_catalog for the catalog repo, api/catalog and ingestion-server/ingestion_server for the API repo).
@AetherUnbound AetherUnbound added 🟩 priority: low Low priority and doesn't need to be rushed 🤖 aspect: dx Concerns developers' experience with the codebase 🧰 goal: internal improvement Improvement that benefits maintainers, not users labels Nov 28, 2022
@dhruvkb
Copy link
Member

dhruvkb commented Nov 29, 2022

The API already has an isort config file .isort.cfg so the catalog can define one in the root too. This need not be blocked on #334.

@AetherUnbound
Copy link
Collaborator Author

Oh! It even has known_first_party defined!

https://github.com/WordPress/openverse-api/blob/55f55de18766854c63c78a39006a5d516744a811/.isort.cfg#L6

Thanks for pointing that out Dhruv 😄

@dhruvkb
Copy link
Member

dhruvkb commented Nov 30, 2022

isort also merges args with the .isort.cfg file so we can put the common parts of the config in the pre-commit args and leave the different parts in the config file. Since #334 is merged, this is ready for work!

@dhruvkb dhruvkb added good first issue New-contributor friendly help wanted Open to participation from the community 🏁 status: ready for work Ready for work labels Nov 30, 2022
@github-project-automation github-project-automation bot moved this to 📋 Backlog in Openverse Backlog Feb 23, 2023
@raiyaj
Copy link
Contributor

raiyaj commented Mar 20, 2023

I'm interested in working on this issue 😊

@raiyaj raiyaj mentioned this issue Mar 20, 2023
Merged
7 tasks
@AetherUnbound AetherUnbound mentioned this issue Mar 20, 2023
Merged
7 tasks
@obulat obulat moved this from 📋 Backlog to 🏗 In progress in Openverse Backlog Mar 28, 2023
@github-project-automation github-project-automation bot moved this from 🏗 In progress to ✅ Done in Openverse Backlog Mar 28, 2023
dhruvkb pushed a commit that referenced this issue Apr 14, 2023
@AetherUnbound
Use pytest-xdist for testing (#337)
760eab5 
* Add pytest-xdist, pytest-sugar to the requirements dev

* Fix s3 tests

* Fix loader sql tests

* Linting

* Allow the loader tests to be flaky since they reference the same file

This also removes some leftover markers from when pytest-socket was removed.

* Move contest, unify some common fixtures

* Fix popularity test_sql

* Optimize jamendo tests

* Make freesound tests a bit faster

* Remove pytest-random-order
@AetherUnbound AetherUnbound added 🧱 stack: api Related to the Django API and removed 🧱 stack: backend labels May 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@raiyaj raiyaj

Labels
🤖 aspect: dx Concerns developers' experience with the codebase 🧰 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 🧱 stack: api Related to the Django API 🏁 status: ready for work Ready for work
Projects
Openverse Backlog
Archived in project
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add isort configuration file raiyaj/openverse-catalog
4 participants
@AetherUnbound @obulat @dhruvkb @raiyaj