Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves configuration resolution and documentation by making config discovery path-based rather than working-directory-based, adds timeout support for the DeepL translator, fixes a bug in markdown paragraph parsing, and adds module docstrings across the codebase.
Key Changes:
- Config resolution now starts from the target path and walks upward, enabling per-folder configurations
- DeepL translator gains configurable timeout (default 5s) to prevent CLI hangs
- Bug fix: markdown paragraph end index calculation now uses
max()instead of conditional - Documentation updated to explain new config/cache resolution behavior
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| fds_dev/config.py | Modified find_config_file() to accept path parameter and search from that location; load_config() now path-aware |
| fds_dev/main.py | Added resolve_cache_path() helper; both commands now call load_config(path) for context-sensitive configuration |
| fds_dev/translator.py | DeepL translator accepts timeout parameter and includes exception details in error messages |
| fds_dev/i18n/code_comment_parser.py | Fixed _mk_markdown_node() to use max(end_idx, start_idx) preventing negative indices |
| fds_dev/*.py | Added minimal module docstrings ("FDS-Dev module.") to all files |
| .pylintrc | New pylint configuration with extensive disabled checks to achieve 10/10 score |
| README.md, README_KR.md | New sections documenting config/cache resolution and DeepL timeout configuration |
| CHANGELOG.md | Added version 0.0.4 entry documenting these changes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| config = DEFAULT_CONFIG.copy() | ||
| config.update(user_config) |
There was a problem hiding this comment.
The config merge at lines 49-50 uses a shallow copy and update, which will completely replace nested dictionaries. For example, if a user only sets language.source: 'ko' in their config, the default language.target: 'en' will be lost. Consider implementing a deep merge to properly combine nested configurations, or document this limitation clearly.
| path_obj = Path(target_path) | ||
| if path_obj.is_dir(): | ||
| return path_obj / '.fds_cache.json' | ||
| parent = path_obj.parent if path_obj.parent.as_posix() else Path('.') |
There was a problem hiding this comment.
The expression path_obj.parent.as_posix() can return an empty string for the current directory, which is falsy, but then the fallback to Path('.') is unnecessary because path_obj.parent already represents the parent directory (even if it's the current directory). Consider simplifying to parent = path_obj.parent if path_obj.parent != Path('.') else Path('.') or just use path_obj.parent directly since it will never be empty for a valid path.
| parent = path_obj.parent if path_obj.parent.as_posix() else Path('.') | |
| parent = path_obj.parent |
| ### 설정 및 캐시 처리 | ||
| - `fds lint`/`fds translate`에 넘긴 경로를 시작점으로 `.fdsrc.yaml`을 탐색합니다. 각 문서 트리마다 별도 규칙을 둘 수 있으며, 작업 디렉터리를 옮길 필요가 없습니다. | ||
| - 린트 캐시(`.fds_cache.json`)는 대상 경로나 파일과 같은 위치에 저장되어 트리별로 스코프가 분리됩니다. | ||
| - `translate`는 자동 언어 감지 시 신뢰도( confidence )를 출력하고, 소스/타깃 언어가 같으면 안전하게 건너뜁니다. |
There was a problem hiding this comment.
The spacing around "confidence" in the Korean text is inconsistent. There's a space before the opening parenthesis but not after. Consider using consistent spacing: either 신뢰도(confidence) or 신뢰도( confidence ) (currently it's 신뢰도( confidence ))
| - `translate`는 자동 언어 감지 시 신뢰도( confidence )를 출력하고, 소스/타깃 언어가 같으면 안전하게 건너뜁니다. | |
| - `translate`는 자동 언어 감지 시 신뢰도(confidence)를 출력하고, 소스/타깃 언어가 같으면 안전하게 건너뜁니다. |
| max-line-length = 100 | ||
|
|
||
| [MESSAGES CONTROL] | ||
| disable = missing-module-docstring,missing-class-docstring,missing-function-docstring,too-few-public-methods,invalid-name,line-too-long,wrong-import-order,broad-exception-caught,redefined-outer-name,unused-argument,unused-import,too-many-instance-attributes,too-many-arguments,too-many-positional-arguments,too-many-locals,unnecessary-pass,disallowed-name |
There was a problem hiding this comment.
The disable list in the pylintrc is extremely broad, disabling many valuable checks including unused-import, unused-argument, missing-function-docstring, and broad-exception-caught. While this might achieve a 10/10 score, it defeats the purpose of using a linter. Consider keeping at least unused-import, unused-argument, and broad-exception-caught enabled to catch real issues.
| disable = missing-module-docstring,missing-class-docstring,missing-function-docstring,too-few-public-methods,invalid-name,line-too-long,wrong-import-order,broad-exception-caught,redefined-outer-name,unused-argument,unused-import,too-many-instance-attributes,too-many-arguments,too-many-positional-arguments,too-many-locals,unnecessary-pass,disallowed-name | |
| disable = missing-module-docstring,missing-class-docstring,missing-function-docstring,too-few-public-methods,invalid-name,line-too-long,wrong-import-order,redefined-outer-name,too-many-instance-attributes,too-many-arguments,too-many-positional-arguments,too-many-locals,unnecessary-pass,disallowed-name |
| """FDS-Dev module.""" | ||
|
|
||
| import hashlib | ||
| import yaml |
There was a problem hiding this comment.
Import of 'yaml' is not used.
| import yaml |
| import yaml | ||
| from typing import List, Dict, Any, Tuple, Optional | ||
|
|
||
| from fds_dev.parser import Document, MarkdownParser |
There was a problem hiding this comment.
Import of 'Document' is not used.
| from fds_dev.parser import Document, MarkdownParser | |
| from fds_dev.parser import MarkdownParser |
|
|
||
| import re | ||
| from dataclasses import dataclass, field | ||
| from typing import Dict, List, Set, Tuple |
There was a problem hiding this comment.
Import of 'Set' is not used.
| from typing import Dict, List, Set, Tuple | |
| from typing import Dict, List, Tuple |
No description provided.