-
Notifications
You must be signed in to change notification settings - Fork 11
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
feat: add utils for creating files #487
Conversation
avoids forgetting to set encryption_status
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM, but a few things to note:
- Rename the
utils.py
toapi.py
(similar to Caluma, where the public python API is also in anapi
module) - Add some docstrings to explain what the functions do
- Add some documentation (README.md or create a new file PYTHON_API.md or similar)
add docs to api
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thought about the whole thing some more, and I think for a "public" API, we need to refine it a bit more. Good work so far though!
alexandria/core/api.py
Outdated
from alexandria.core import models | ||
|
||
|
||
def create_document_file(user, group, document_attributes, file_attributes): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since there are some required arguments, it may make sense to have them listed directly in the arguments. That way, it's clear without even needing to document them.
The file arguments could be renamed, so as to show which ones are required as well. Maybe something like this:
def create_document_file(user, group, document_attributes, file_attributes): | |
def create_document_file( | |
user, | |
group, | |
category, | |
title, | |
file_name, | |
file_content, | |
mime_type, | |
file_size, | |
additional_attributes=None, | |
additional_file_attributes=None | |
): | |
additional_attributes = additional_attributes or {} | |
additional_file_attributes = additional_file_attributes or {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest renaming the **attributes
/ **file_attributes
to **additional_attributes
, to further show that they're additional to the regular ones.
Otherwise, the PR looks good now, and ready to merge
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Yet another thought: some of the functionality could probably be put into django signal handlers instead ( |
Could be considered for a seperate PR, as that would introduce some more code overhaul. We can open an issue for this. |
Nice work! Please squash before merge though, we don't need all intra-PR fixes in the history |
avoids forgetting to set encryption_status