Uh oh!
There was an error while loading. Please reload this page.
fix(oocana): use dict mapping for compression_suffix instead of if-elif chain - #457
fix(oocana): use dict mapping for compression_suffix instead of if-elif chain#457leavesster wants to merge 1 commit into
Conversation
Replace if-elif chain with a dictionary mapping for cleaner code. Also fix potential KeyError by using .get() method. Changes: - Add COMPRESSION_SUFFIXES constant mapping method to suffix - Use .get() to safely access 'method' key - Add comprehensive unit tests for compression_suffix
总体说明本次变更通过引入 COMPRESSION_SUFFIXES 映射表，将压缩后缀逻辑从多个条件分支重构为单一映射查询机制。同时新增了一个全面的测试模块，用于验证压缩后缀和压缩选项在各种场景下的行为。 变更内容
代码审查工作量估算🎯 3 (中等复杂度) | ⏱️ ~20 分钟 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
Pull request overview
Refactors compression_suffix() to use a dictionary mapping for compression method → file suffix (instead of an if/elif chain) and adds unit tests to validate behavior across supported and edge-case methods.
Changes:
- Introduced
COMPRESSION_SUFFIXESmapping and updatedcompression_suffix()to use.get()-based lookup with defaults. - Updated
compression_suffix()docstring to match actual default behavior (.pkl). - Added unit tests covering supported methods and fallback behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
oocana/oocana/serialization.py | Replaces if/elif suffix selection with a mapping constant and simplified lookup logic. |
oocana/tests/test_serialization.py | Adds tests for compression_suffix() and compression_options() behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -0,0 +1,164 @@ | |||
| import unittest | |||
| from unittest.mock import MagicMock, patch | |||
There was a problem hiding this comment.
patch is imported but never used in this test module; removing it avoids lint warnings and keeps the test file tidy.
| fromunittest.mockimportMagicMock, patch | |
| fromunittest.mockimportMagicMock |
| method = compression.get("method") | ||
| if method is None: |
There was a problem hiding this comment.
COMPRESSION_SUFFIXES.get(method, ...) assumes method is a hashable key. If the options file contains a non-string (e.g., a list/dict from valid-but-unexpected JSON), this will raise TypeError and crash, whereas the previous if/elif chain would safely fall back. Consider guarding with isinstance(method, str) (and/or isinstance(compression, dict)) before doing the dict lookup, otherwise return the default suffix.
| method=compression.get("method") | |
| ifmethodisNone: | |
| # Ensure we have a dictionary before accessing keys. Malformed or unexpected | |
| # JSON in the options file may result in a non-dict value here. | |
| ifnotisinstance(compression, dict): | |
| return".pkl" | |
| method=compression.get("method") | |
| # Guard against non-string (and thus potentially unhashable) methods. | |
| ifnotisinstance(method, str): |
| # Mapping from compression method to file suffix | ||
| COMPRESSION_SUFFIXES = { | ||
| "zip": ".zip", | ||
| "gzip": ".gz", | ||
| "bz2": ".bz2", | ||
| "zstd": ".zst", | ||
| "xz": ".xz", | ||
| "tar": ".tar", | ||
| } |
There was a problem hiding this comment.
The compression methods are now duplicated in three places (SUPPORTED_COMPRESSION_METHODS, the CompressionOptions Literal, and COMPRESSION_SUFFIXES). This can drift over time (e.g., a method added to SUPPORTED_COMPRESSION_METHODS but missing here would silently fall back to .pkl). Consider making one source of truth (e.g., derive SUPPORTED_COMPRESSION_METHODS from COMPRESSION_SUFFIXES.keys()), and keep the type Literal in sync with that constant.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@oocana/oocana/serialization.py`:
- Around line 87-96: compression_options 返回的 compression 可能不是 dict，从而在
compression.get("method") 处抛出 AttributeError；在函数/片段中（使用
compression_options、compression、method、COMPRESSION_SUFFIXES 的地方）先对 compression
做类型保护（例如 isinstance(compression, dict) 或
collections.abc.Mapping），若不是合适的映射类型则直接返回默认后缀 ".pkl"；若是映射再安全读取 method 并用
COMPRESSION_SUFFIXES.get(method, ".pkl") 返回结果。
🧹 Nitpick comments (2)
oocana/oocana/serialization.py (1)
72-80: 避免与SUPPORTED_COMPRESSION_METHODS重复来源，防止漂移
目前方法列表在SUPPORTED_COMPRESSION_METHODS和COMPRESSION_SUFFIXES中重复维护，后续新增/删除方法容易不同步。建议以COMPRESSION_SUFFIXES作为单一来源生成列表。若该常量希望对外公开，也可同步加入__all__。🔧 参考改法（统一来源）
-SUPPORTED_COMPRESSION_METHODS = ["zip", "gzip", "bz2", "zstd", "xz", "tar"]+# Mapping from compression method to file suffix+COMPRESSION_SUFFIXES = {+ "zip": ".zip",+ "gzip": ".gz",+ "bz2": ".bz2",+ "zstd": ".zst",+ "xz": ".xz",+ "tar": ".tar",+}+SUPPORTED_COMPRESSION_METHODS = list(COMPRESSION_SUFFIXES.keys())oocana/tests/test_serialization.py (1)
29-119: 建议参数化这些重复的后缀测试zip/gzip/bz2/zstd/xz/tar的测试结构完全一致，后续维护成本偏高。可以用subTest+ 参数表合并为一个测试。♻️ 参考改法（参数化）
- def test_compression_suffix_zip(self):- """Test compression suffix for zip method."""- from oocana.oocana.serialization import compression_suffix, COMPRESSION_OPTIONS_FILE-- with open(os.path.join(self.temp_dir, COMPRESSION_OPTIONS_FILE), "w") as f:- json.dump({"method": "zip"}, f)-- result = compression_suffix(self.mock_context)- self.assertEqual(result, ".zip")-- def test_compression_suffix_gzip(self):- """Test compression suffix for gzip method."""- from oocana.oocana.serialization import compression_suffix, COMPRESSION_OPTIONS_FILE-- with open(os.path.join(self.temp_dir, COMPRESSION_OPTIONS_FILE), "w") as f:- json.dump({"method": "gzip"}, f)-- result = compression_suffix(self.mock_context)- self.assertEqual(result, ".gz")-- ...-- def test_compression_suffix_tar(self):- """Test compression suffix for tar method."""- from oocana.oocana.serialization import compression_suffix, COMPRESSION_OPTIONS_FILE-- with open(os.path.join(self.temp_dir, COMPRESSION_OPTIONS_FILE), "w") as f:- json.dump({"method": "tar"}, f)-- result = compression_suffix(self.mock_context)- self.assertEqual(result, ".tar")+ def test_compression_suffix_methods(self):+ """Test compression suffix for supported methods."""+ from oocana.oocana.serialization import compression_suffix, COMPRESSION_OPTIONS_FILE++ cases = {+ "zip": ".zip",+ "gzip": ".gz",+ "bz2": ".bz2",+ "zstd": ".zst",+ "xz": ".xz",+ "tar": ".tar",+ }+ for method, expected in cases.items():+ with self.subTest(method=method):+ with open(os.path.join(self.temp_dir, COMPRESSION_OPTIONS_FILE), "w") as f:+ json.dump({"method": method}, f)+ self.assertEqual(compression_suffix(self.mock_context), expected)
| compression = compression_options(context) | ||
| if compression is None or compression["method"] is None: | ||
| if compression is None: | ||
| return ".pkl" | ||
| method = compression["method"] | ||
| if method == "zip": | ||
| return ".zip" | ||
| elif method == "gzip": | ||
| return ".gz" | ||
| elif method == "bz2": | ||
| return ".bz2" | ||
| elif method == "zstd": | ||
| return ".zst" | ||
| elif method == "xz": | ||
| return ".xz" | ||
| elif method == "tar": | ||
| return ".tar" | ||
| else: | ||
| return ".pkl" # Default case if method is not recognized No newline at end of file | ||
| method = compression.get("method") | ||
| if method is None: | ||
| return ".pkl" | ||
| return COMPRESSION_SUFFIXES.get(method, ".pkl") No newline at end of file |
There was a problem hiding this comment.
防止压缩配置为非 dict 时触发 AttributeErrorcompression_options 读取的是任意 JSON，若文件内容为合法但非 dict（如字符串/列表），compression.get(...) 会直接报错。建议加类型保护并回退到默认后缀。
🛡️ 参考修复
compression = compression_options(context)
- if compression is None:+ if not isinstance(compression, dict):
return ".pkl"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| compression=compression_options(context) | |
| ifcompressionisNoneorcompression["method"] isNone: | |
| ifcompressionisNone: | |
| return".pkl" | |
| method=compression["method"] | |
| ifmethod=="zip": | |
| return".zip" | |
| elifmethod=="gzip": | |
| return".gz" | |
| elifmethod=="bz2": | |
| return".bz2" | |
| elifmethod=="zstd": | |
| return".zst" | |
| elifmethod=="xz": | |
| return".xz" | |
| elifmethod=="tar": | |
| return".tar" | |
| else: | |
| return".pkl"# Default case if method is not recognized | |
| \ Nonewlineatendoffile | |
| method=compression.get("method") | |
| ifmethodisNone: | |
| return".pkl" | |
| returnCOMPRESSION_SUFFIXES.get(method, ".pkl") | |
| compression=compression_options(context) | |
| ifnotisinstance(compression, dict): | |
| return".pkl" | |
| method=compression.get("method") | |
| ifmethodisNone: | |
| return".pkl" | |
| returnCOMPRESSION_SUFFIXES.get(method, ".pkl") |
🤖 Prompt for AI Agents
In `@oocana/oocana/serialization.py` around lines 87 - 96, compression_options 返回的
compression 可能不是 dict，从而在 compression.get("method") 处抛出
AttributeError；在函数/片段中（使用
compression_options、compression、method、COMPRESSION_SUFFIXES 的地方）先对 compression
做类型保护（例如 isinstance(compression, dict) 或
collections.abc.Mapping），若不是合适的映射类型则直接返回默认后缀 ".pkl"；若是映射再安全读取 method 并用
COMPRESSION_SUFFIXES.get(method, ".pkl") 返回结果。
Summary
COMPRESSION_SUFFIXESdictionary mapping.get()to safely handle missing keyscompression_suffix()functionProblem
Original code had long if-elif chain with potential KeyError risk:
Solution
Test Plan
tests/test_serialization.pywith coverage for all compression methods