-
Notifications
You must be signed in to change notification settings - Fork 0
feat: add folder loading support for bulk JSON imports #7
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
Conversation
devjerry0
commented
Dec 15, 2025
- Add load_folder() function to load all JSON files from a directory
- Automatically ignores non-JSON files in the folder
- Loads files in sorted order for predictable behavior
- Comprehensive error handling:
- FileNotFoundError for missing folders
- NotADirectoryError for file paths
- ValueError for folders with no JSON files
- Add 5 new tests covering all edge cases
- Expose via contractions.load_folder() API
- Add load_folder() function to load all JSON files from a directory - Automatically ignores non-JSON files in the folder - Loads files in sorted order for predictable behavior - Comprehensive error handling: - FileNotFoundError for missing folders - NotADirectoryError for file paths - ValueError for folders with no JSON files - Add 5 new tests covering all edge cases - Expose via contractions.load_folder() API
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
- Document load_folder() in usage examples - Add API reference section with parameters and exceptions - Update new features list
Instead of calling add_custom_dict() for each file (iterating through all matchers N times), merge all dictionaries first then add once. For 10 files: 10 matcher iterations → 1 matcher iteration
Improved separation of concerns: - file_loader.py: handles all file I/O operations - extension.py: only handles adding contractions to matchers Added load_dict_from_file() and load_dict_from_folder() to file_loader.py that return data. Extension functions now delegate to these and add to matchers.
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.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
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.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def test_load_folder() -> None: | ||
| with tempfile.TemporaryDirectory() as temp_dir: | ||
| file1 = os.path.join(temp_dir, "dict1.json") | ||
| file2 = os.path.join(temp_dir, "dict2.json") | ||
|
|
||
| with open(file1, "w", encoding="utf-8") as f: | ||
| json.dump({"foldertest1": "folder test one", "foldertest2": "folder test two"}, f) | ||
|
|
||
| with open(file2, "w", encoding="utf-8") as f: | ||
| json.dump({"foldertest3": "folder test three"}, f) | ||
|
|
||
| contractions.load_folder(temp_dir) | ||
|
|
||
| assert contractions.expand("foldertest1") == "folder test one" | ||
| assert contractions.expand("foldertest2") == "folder test two" | ||
| assert contractions.expand("foldertest3") == "folder test three" | ||
|
|
||
|
|
||
| def test_load_folder_ignores_non_json() -> None: | ||
| with tempfile.TemporaryDirectory() as temp_dir: | ||
| json_file = os.path.join(temp_dir, "valid.json") | ||
| txt_file = os.path.join(temp_dir, "ignored.txt") | ||
|
|
||
| with open(json_file, "w", encoding="utf-8") as f: | ||
| json.dump({"validkey": "valid value"}, f) | ||
|
|
||
| with open(txt_file, "w", encoding="utf-8") as f: | ||
| f.write("this should be ignored") | ||
|
|
||
| contractions.load_folder(temp_dir) | ||
| assert contractions.expand("validkey") == "valid value" | ||
|
|
||
|
|
||
| def test_load_folder_not_found() -> None: | ||
| with pytest.raises(FileNotFoundError, match="Folder not found"): | ||
| contractions.load_folder("/nonexistent/folder/path") | ||
|
|
||
|
|
||
| def test_load_folder_not_directory() -> None: | ||
| with tempfile.NamedTemporaryFile(mode="w", suffix=".json", delete=False, encoding="utf-8") as f: | ||
| json.dump({"test": "data"}, f) | ||
| temp_path = f.name | ||
|
|
||
| try: | ||
| with pytest.raises(NotADirectoryError, match="not a directory"): | ||
| contractions.load_folder(temp_path) | ||
| finally: | ||
| os.unlink(temp_path) | ||
|
|
||
|
|
||
| def test_load_folder_no_json_files() -> None: | ||
| with tempfile.TemporaryDirectory() as temp_dir: | ||
| txt_file = os.path.join(temp_dir, "file.txt") | ||
| with open(txt_file, "w", encoding="utf-8") as f: | ||
| f.write("no json here") | ||
|
|
||
| with pytest.raises(ValueError, match="No JSON files found"): | ||
| contractions.load_folder(temp_dir) | ||
|
|
Copilot
AI
Dec 15, 2025
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.
The test suite is missing coverage for the case where a folder contains invalid JSON files. While there's a test for invalid JSON in load_file() (test_load_file_invalid_json), there's no corresponding test for load_folder() handling malformed JSON. This could lead to unclear error behavior when a folder contains files with invalid JSON syntax.
| def test_load_folder() -> None: | ||
| with tempfile.TemporaryDirectory() as temp_dir: | ||
| file1 = os.path.join(temp_dir, "dict1.json") | ||
| file2 = os.path.join(temp_dir, "dict2.json") | ||
|
|
||
| with open(file1, "w", encoding="utf-8") as f: | ||
| json.dump({"foldertest1": "folder test one", "foldertest2": "folder test two"}, f) | ||
|
|
||
| with open(file2, "w", encoding="utf-8") as f: | ||
| json.dump({"foldertest3": "folder test three"}, f) | ||
|
|
||
| contractions.load_folder(temp_dir) | ||
|
|
||
| assert contractions.expand("foldertest1") == "folder test one" | ||
| assert contractions.expand("foldertest2") == "folder test two" | ||
| assert contractions.expand("foldertest3") == "folder test three" | ||
|
|
||
|
|
||
| def test_load_folder_ignores_non_json() -> None: | ||
| with tempfile.TemporaryDirectory() as temp_dir: | ||
| json_file = os.path.join(temp_dir, "valid.json") | ||
| txt_file = os.path.join(temp_dir, "ignored.txt") | ||
|
|
||
| with open(json_file, "w", encoding="utf-8") as f: | ||
| json.dump({"validkey": "valid value"}, f) | ||
|
|
||
| with open(txt_file, "w", encoding="utf-8") as f: | ||
| f.write("this should be ignored") | ||
|
|
||
| contractions.load_folder(temp_dir) | ||
| assert contractions.expand("validkey") == "valid value" | ||
|
|
||
|
|
||
| def test_load_folder_not_found() -> None: | ||
| with pytest.raises(FileNotFoundError, match="Folder not found"): | ||
| contractions.load_folder("/nonexistent/folder/path") | ||
|
|
||
|
|
||
| def test_load_folder_not_directory() -> None: | ||
| with tempfile.NamedTemporaryFile(mode="w", suffix=".json", delete=False, encoding="utf-8") as f: | ||
| json.dump({"test": "data"}, f) | ||
| temp_path = f.name | ||
|
|
||
| try: | ||
| with pytest.raises(NotADirectoryError, match="not a directory"): | ||
| contractions.load_folder(temp_path) | ||
| finally: | ||
| os.unlink(temp_path) | ||
|
|
||
|
|
||
| def test_load_folder_no_json_files() -> None: | ||
| with tempfile.TemporaryDirectory() as temp_dir: | ||
| txt_file = os.path.join(temp_dir, "file.txt") | ||
| with open(txt_file, "w", encoding="utf-8") as f: | ||
| f.write("no json here") | ||
|
|
||
| with pytest.raises(ValueError, match="No JSON files found"): | ||
| contractions.load_folder(temp_dir) | ||
|
|
Copilot
AI
Dec 15, 2025
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.
The test suite is missing coverage for the case where a folder contains JSON files that are not dictionaries (e.g., arrays, strings). While there's a test for non-dict JSON in load_file() (test_load_file_non_dict), there's no corresponding test for load_folder() when encountering files with valid JSON that isn't a dictionary format.
| def test_load_folder() -> None: | ||
| with tempfile.TemporaryDirectory() as temp_dir: | ||
| file1 = os.path.join(temp_dir, "dict1.json") | ||
| file2 = os.path.join(temp_dir, "dict2.json") | ||
|
|
||
| with open(file1, "w", encoding="utf-8") as f: | ||
| json.dump({"foldertest1": "folder test one", "foldertest2": "folder test two"}, f) | ||
|
|
||
| with open(file2, "w", encoding="utf-8") as f: | ||
| json.dump({"foldertest3": "folder test three"}, f) | ||
|
|
||
| contractions.load_folder(temp_dir) | ||
|
|
||
| assert contractions.expand("foldertest1") == "folder test one" | ||
| assert contractions.expand("foldertest2") == "folder test two" | ||
| assert contractions.expand("foldertest3") == "folder test three" | ||
|
|
||
|
|
||
| def test_load_folder_ignores_non_json() -> None: | ||
| with tempfile.TemporaryDirectory() as temp_dir: | ||
| json_file = os.path.join(temp_dir, "valid.json") | ||
| txt_file = os.path.join(temp_dir, "ignored.txt") | ||
|
|
||
| with open(json_file, "w", encoding="utf-8") as f: | ||
| json.dump({"validkey": "valid value"}, f) | ||
|
|
||
| with open(txt_file, "w", encoding="utf-8") as f: | ||
| f.write("this should be ignored") | ||
|
|
||
| contractions.load_folder(temp_dir) | ||
| assert contractions.expand("validkey") == "valid value" | ||
|
|
||
|
|
||
| def test_load_folder_not_found() -> None: | ||
| with pytest.raises(FileNotFoundError, match="Folder not found"): | ||
| contractions.load_folder("/nonexistent/folder/path") | ||
|
|
||
|
|
||
| def test_load_folder_not_directory() -> None: | ||
| with tempfile.NamedTemporaryFile(mode="w", suffix=".json", delete=False, encoding="utf-8") as f: | ||
| json.dump({"test": "data"}, f) | ||
| temp_path = f.name | ||
|
|
||
| try: | ||
| with pytest.raises(NotADirectoryError, match="not a directory"): | ||
| contractions.load_folder(temp_path) | ||
| finally: | ||
| os.unlink(temp_path) | ||
|
|
||
|
|
||
| def test_load_folder_no_json_files() -> None: | ||
| with tempfile.TemporaryDirectory() as temp_dir: | ||
| txt_file = os.path.join(temp_dir, "file.txt") | ||
| with open(txt_file, "w", encoding="utf-8") as f: | ||
| f.write("no json here") | ||
|
|
||
| with pytest.raises(ValueError, match="No JSON files found"): | ||
| contractions.load_folder(temp_dir) | ||
|
|
Copilot
AI
Dec 15, 2025
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.
Missing test coverage for verifying behavior when JSON files in a folder have overlapping keys. The implementation uses dict.update() which will silently overwrite earlier values with later ones based on sorted filename order. This behavior should be explicitly tested to document the expected behavior.
| **Raises:** | ||
| - `FileNotFoundError`: If the folder doesn't exist | ||
| - `NotADirectoryError`: If the path is a file, not a directory | ||
| - `ValueError`: If no JSON files are found in the folder |
Copilot
AI
Dec 15, 2025
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.
The documentation for load_folder() is incomplete. It should also document that json.JSONDecodeError can be raised when a JSON file in the folder contains invalid JSON syntax, similar to how load_file() documents this exception. This is important for users to understand all possible error conditions.
| - `ValueError`: If no JSON files are found in the folder | |
| - `ValueError`: If no JSON files are found in the folder | |
| - `json.JSONDecodeError`: If any JSON file contains invalid JSON |
| ### `load_folder(folderpath)` | ||
|
|
||
| Loads custom contractions from all JSON files in a directory. Non-JSON files are automatically ignored. | ||
|
|
Copilot
AI
Dec 15, 2025
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.
The documentation should clarify the behavior when multiple JSON files contain the same keys. Currently, later files (in sorted order) will overwrite values from earlier files. This merge behavior should be documented so users understand how conflicts are resolved.
| **Note:** If multiple JSON files contain the same contraction keys, values from later files (in sorted order) will overwrite those from earlier files. This means the final mapping will reflect the values from the last file containing each key. |