feat: drop file type restriction for upload #236
Conversation
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||
There was a problem hiding this comment.
Copilot reviewed 5 out of 12 changed files in this pull request and generated no comments.
Files not reviewed (7)
- deepset_cloud_sdk/_utils/constants.py: Evaluated as low risk
- deepset_cloud_sdk/_service/files_service.py: Evaluated as low risk
- deepset_cloud_sdk/workflows/sync_client/files.py: Evaluated as low risk
- tests/integration/service/test_integration_files_service.py: Evaluated as low risk
- tests/unit/api/test_files.py: Evaluated as low risk
- tests/unit/service/test_files_service.py: Evaluated as low risk
- deepset_cloud_sdk/cli.py: Evaluated as low risk
Comments suppressed due to low confidence (1)
deepset_cloud_sdk/workflows/async_client/files.py:149
- Ensure that the removal of the file type restriction is covered by tests to avoid unintended behavior.
:param desired_file_types: A list of allowed file types to upload. If not provided, all files are uploaded.
| @staticmethod | ||
| def _preprocess_paths( | ||
| paths: List[Path], | ||
| spinner: yaspin.Spinner = None, | ||
| recursive: bool = False, | ||
| desired_file_types: Optional[List[str]] = None, | ||
| desired_file_types: List[str] | None = None, |
There was a problem hiding this comment.
Nit: we could use sth like the following so that you can get rid of all the None checks
class AlwaysContains:
def __contains__(self, item):
return True
fake_list = AlwaysContains()
print(42 in fake_list) # True
print("hello" in fake_list) # True
print(None in fake_list) # TrueThere was a problem hiding this comment.
(it's in my opinion easy to miss the check somewhere)
There was a problem hiding this comment.
you mean replacing the native List objects with a class ? I would need to adjust it also within the typer Argument => CLI parsing, but might work.
There was a problem hiding this comment.
More replacing the None with an object behaving like a list (basically simulating the null pattern https://refactoring.guru/introduce-null-object
This pull request removes the
SUPPORTED_TYPE_SUFFIXESconstant and updates the codebase to handle file type validation differently. The changes focus on simplifying the file upload process and removing redundant checks.Removal of
SUPPORTED_TYPE_SUFFIXES:deepset_cloud_sdk/_api/files.py: Removed theSUPPORTED_TYPE_SUFFIXESimport and the file type validation check indirect_upload_in_memory. [1] [2]deepset_cloud_sdk/_service/files_service.py: Removed theSUPPORTED_TYPE_SUFFIXESimport and associated validation checks in various methods such asupload_file_paths,_validate_file_paths, and_preprocess_paths. [1] [2] [3] [4] [5] [6]deepset_cloud_sdk/_utils/constants.py: Completely removed theSUPPORTED_TYPE_SUFFIXESconstant.Updates to file upload methods:
deepset_cloud_sdk/cli.py: Removed the defaultuse_typevalue in theuploadfunction.deepset_cloud_sdk/workflows/async_client/files.py: Updated theuploadfunction to remove default file types and handle all files if no specific types are provided. [1] [2]deepset_cloud_sdk/workflows/sync_client/files.py: Similar updates to the synchronousuploadfunction to handle all files if no specific types are provided. [1] [2]Test updates:
tests/integration/service/test_integration_files_service.py: Updated tests to specify desired file types directly instead of usingSUPPORTED_TYPE_SUFFIXES. [1] [2] [3]tests/unit/api/test_files.py: Removed the test for invalid file type names.tests/unit/service/test_files_service.py: Updated tests to specify desired file types directly and removed tests related toSUPPORTED_TYPE_SUFFIXES. [1] [2] [3] [4] [5]tests/unit/test_cli.py: Updated tests to handledesired_file_typesasNoneby default. [1] [2]tests/unit/workflows/async_client/test_async_workflow_files.py: Updated theuploadtest to handledesired_file_typesasNoneby default. [1] [2]