feat: added a list files in dir method #1
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new list_files_in_dir method to the UFAClient class to support directory listing functionality. The implementation calls a UFA directory listing endpoint and returns paginated results with optional continuation tokens.
- Adds
list_files_in_dirmethod toUFAClientwith support for pagination and result limits - Implements integration test to verify directory listing functionality
- Adds documentation explaining the API usage and endpoint structure
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/ufa/client.py | Adds list_files_in_dir method with percent-encoding of directory paths and pagination support |
| tests/test_client.py | Adds integration test that uploads fixtures and verifies they appear in directory listing |
| docs/ufa_client.md | Provides usage examples and documents the HTTP endpoint mapping |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| query_parts.append(f"maxKeys={max_keys}") | ||
| url = base if not query_parts else f"{base}?{'&'.join(query_parts)}" | ||
|
|
||
| async with httpx.AsyncClient() as client: |
There was a problem hiding this comment.
The list_files_in_dir method creates a new httpx.AsyncClient for each call without specifying a timeout, unlike upload_file which sets explicit timeouts. This could lead to indefinite hangs if the server is unresponsive. Consider adding a timeout configuration similar to the upload method.
| async with httpx.AsyncClient() as client: | |
| async with httpx.AsyncClient(timeout=httpx.Timeout(60.0, connect=10.0)) as client: |
| async with httpx.AsyncClient() as client: | ||
| response = await self._make_request(client, url) | ||
| resp_ct = response.headers.get("content-type", "") | ||
| if "application/json" in resp_ct: | ||
| return response.json() | ||
| # Fallback: attempt to decode JSON even if content-type is missing | ||
| try: | ||
| return response.json() | ||
| except ValueError: | ||
| logger.error("Unexpected non-JSON response for directory listing") | ||
| raise UFAError( |
There was a problem hiding this comment.
The fallback logic at lines 277-278 is redundant. If the content-type check at line 274 passes, the method returns at line 275 and never reaches the fallback. If it fails, the try-except block will execute the same response.json() call. Consider simplifying to a single try-except block that handles both cases.
| # Fallback: attempt to decode JSON even if content-type is missing | ||
| try: | ||
| return response.json() | ||
| except ValueError: |
There was a problem hiding this comment.
The exception handling catches ValueError, but response.json() can raise json.JSONDecodeError (which inherits from ValueError) or other exceptions. Consider explicitly catching json.JSONDecodeError for clarity, or handling potential httpx exceptions that could occur during JSON parsing.
| def test_list_files_in_dir(ufa_client: UFAClient) -> None: | ||
| """Upload fixtures and verify they appear in directory listing.""" | ||
| local_files = _iter_fixture_files() | ||
| if not local_files: | ||
| pytest.skip("no fixtures to upload") | ||
|
|
||
| # Ensure all fixtures are present remotely under REMOTE_PREFIX | ||
| for p in local_files: | ||
| asyncio.run( | ||
| ufa_client.upload_file( | ||
| local_file_path=str(p), | ||
| remote_file_path=_remote_path_for(p), | ||
| ) | ||
| ) | ||
|
|
||
| # List the directory and assert each filename is present | ||
| listing = asyncio.run( | ||
| ufa_client.list_files_in_dir(directory_path=REMOTE_PREFIX.rstrip("/")) | ||
| ) |
There was a problem hiding this comment.
Multiple asyncio.run() calls in a loop create and tear down an event loop for each iteration. This is inefficient and could cause issues with async resource management. Consider using a single asyncio.run() call with asyncio.gather() to upload all files concurrently, or define the test as an async function and use pytest-asyncio.
| def test_list_files_in_dir(ufa_client: UFAClient) -> None: | |
| """Upload fixtures and verify they appear in directory listing.""" | |
| local_files = _iter_fixture_files() | |
| if not local_files: | |
| pytest.skip("no fixtures to upload") | |
| # Ensure all fixtures are present remotely under REMOTE_PREFIX | |
| for p in local_files: | |
| asyncio.run( | |
| ufa_client.upload_file( | |
| local_file_path=str(p), | |
| remote_file_path=_remote_path_for(p), | |
| ) | |
| ) | |
| # List the directory and assert each filename is present | |
| listing = asyncio.run( | |
| ufa_client.list_files_in_dir(directory_path=REMOTE_PREFIX.rstrip("/")) | |
| ) | |
| @pytest.mark.asyncio | |
| async def test_list_files_in_dir(ufa_client: UFAClient) -> None: | |
| """Upload fixtures and verify they appear in directory listing.""" | |
| local_files = _iter_fixture_files() | |
| if not local_files: | |
| pytest.skip("no fixtures to upload") | |
| # Ensure all fixtures are present remotely under REMOTE_PREFIX | |
| await asyncio.gather(*[ | |
| ufa_client.upload_file( | |
| local_file_path=str(p), | |
| remote_file_path=_remote_path_for(p), | |
| ) | |
| for p in local_files | |
| ]) | |
| # List the directory and assert each filename is present | |
| listing = await ufa_client.list_files_in_dir(directory_path=REMOTE_PREFIX.rstrip("/")) |
No description provided.