Restructure utils.py to its own package
#915
Conversation
| from .validators import state_validator, url_validator | ||
|
|
||
|
|
||
| db = SQLAlchemy() | ||
| model_encoding = "utf-8" |
There was a problem hiding this comment.
We could remove this now that the constant isn't a circular reference.
|
Gonna run the tests on my local sometime tomorrow. |
| @@ -89,5 +89,6 @@ help: ## Print this message and exit | |||
| | sort \ | |||
| | column -s ':' -t | |||
|
|
|||
| .PHONY: attach | |||
There was a problem hiding this comment.
Added a missing PHONY.
There was a problem hiding this comment.
Firstly, Thank you very much for doing this!
One nit: Was it intentional to change utils.py to util, getting rid of the plural? It appears that generally we are leaning towards the plural in the naming here.
Finally, I did remember an issue that we ran into in the past, that prevented us from using absolute imports. I tested it locally and I believe it is still an issue, so I created #916 to change that. So in short, I #916 should be merged first and then this one should be fine :)
OpenOversight/app/util/general.py
Outdated
| ) | ||
|
|
||
|
|
||
| def get_or_create(session, model, defaults=None, **kwargs): |
There was a problem hiding this comment.
I am wondering if this get_or_create could be getting its own file or something? Since it's also imported from util/forms.py seemingly increasing the change of running into more circular imports in the future. But if you think it's fine, I am okay leaving it as it is
There was a problem hiding this comment.
If we're good now, I think we can keep it where it's at. I'll add a note to the top of it tho signaling that it could be something we move to its own file should a circular import arise.
|
That's good to know! I'll make the change from Edit: Done. ✅ |
| # Operating system files | ||
| **/.DS_Store |
There was a problem hiding this comment.
I was getting this file if I opened up anything in finder on my comp.
| @@ -15,7 +15,7 @@ | |||
| sys.path.insert(0, os.path.dirname(current_app.root_path)) | |||
|
|
|||
| from app.models import Face, db # noqa: E402 | |||
| from app.utils import crop_image # noqa: E402 | |||
| from app.utils.cloud import crop_image # noqa: E402 | |||
There was a problem hiding this comment.
I felt weird changing a migration file, but it seems it was necessary.
* Move utils to its own package * Move constants to own file * Update Makefile * Update forms.py * Update test_utils.py * Use absolute imports * Update 59e9993c169c_change_faces_to_thumbnails.py * Update .gitignore
* Move utils to its own package * Move constants to own file * Update Makefile * Update forms.py * Update test_utils.py * Use absolute imports * Update 59e9993c169c_change_faces_to_thumbnails.py * Update .gitignore
* Move utils to its own package * Move constants to own file * Update Makefile * Update forms.py * Update test_utils.py * Use absolute imports * Update 59e9993c169c_change_faces_to_thumbnails.py * Update .gitignore
* Move utils to its own package * Move constants to own file * Update Makefile * Update forms.py * Update test_utils.py * Use absolute imports * Update 59e9993c169c_change_faces_to_thumbnails.py * Update .gitignore
Status
Ready for review.
Description of Changes
Changed
utils.pyfile from a gigantic mono-file to a package with more specialized files.Fixes: #913
Tests and linting
developpre-commitchecks pass