Skip to content

Commit 570aea1

Browse files
authored
Merge pull request #1355 from PyThaiNLP/copilot/tidy-code-for-consistency
Tidy exception forwarding, warning consistency, and document conventions
2 parents 3b4fe31 + 4905010 commit 570aea1

19 files changed

Lines changed: 194 additions & 59 deletions

File tree

CONTRIBUTING.md

Lines changed: 132 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,138 @@ Please refer to our
5353
[empty-line]: https://stackoverflow.com/questions/5813311/no-newline-at-end-of-file#5813359
5454
[posix]: https://stackoverflow.com/questions/729692/why-should-text-files-end-with-a-newline
5555

56+
### Error messages, warnings, and exception handling
57+
58+
Clear, consistent, and parseable error and warning messages help users
59+
debug problems and allow tooling to parse and filter output.
60+
Follow these conventions in all PyThaiNLP code.
61+
62+
#### Exception types
63+
64+
Use the most specific built-in exception type for each situation:
65+
66+
| Situation | Exception type |
67+
| --- | --- |
68+
| Missing optional dependency at import time | `ImportError` or `ModuleNotFoundError` |
69+
| Invalid argument value | `ValueError` |
70+
| Wrong argument type | `TypeError` |
71+
| Required file not found | `FileNotFoundError` |
72+
| I/O or OS-level failure | `OSError` |
73+
| Runtime failure with no more-specific type | `RuntimeError` |
74+
| Feature not yet implemented | `NotImplementedError` |
75+
76+
#### Exception message format
77+
78+
Write messages as complete sentences.
79+
80+
- End each sentence with a period.
81+
- Identify the offending value, name, or path explicitly.
82+
- For a missing optional dependency, include the `pip install` command:
83+
84+
```text
85+
<Package> is not installed. Install it with: pip install <package>
86+
```
87+
88+
If the package is a PyThaiNLP optional-dependency group, name the extra:
89+
90+
```text
91+
<Package> is required for this feature.
92+
Install it with: pip install pythainlp[<extra>]
93+
```
94+
95+
- For an invalid argument value, name the parameter and show the received value:
96+
97+
```text
98+
<param> must be <description>; got {value!r}
99+
```
100+
101+
#### Exception forwarding (chaining)
102+
103+
Always chain exceptions with `from` when raising a new exception inside an
104+
`except` block, so the full traceback and original cause are preserved:
105+
106+
```python
107+
# Correct: chain to the original exception.
108+
try:
109+
import some_package
110+
except ImportError as e:
111+
raise ImportError(
112+
"some_package is not installed. Install it with: pip install some_package"
113+
) from e
114+
115+
# Correct: suppress chain with `from None` only when the original
116+
# exception is irrelevant noise that would confuse the user.
117+
raise ValueError("Invalid configuration value.") from None
118+
```
119+
120+
Never use `raise e` to re-raise a caught exception. Use bare `raise` instead,
121+
which preserves the original traceback:
122+
123+
```python
124+
# Correct: bare re-raise preserves traceback.
125+
except Exception:
126+
log_or_print_something()
127+
raise
128+
129+
# Incorrect: `raise e` creates a new traceback starting at this line.
130+
except Exception as e:
131+
log_or_print_something()
132+
raise e # do not do this
133+
```
134+
135+
When re-raising as a *different* exception type, always supply `from`:
136+
137+
```python
138+
# Correct.
139+
except OSError as e:
140+
raise RuntimeError(f"Failed to read model file: {e}") from e
141+
142+
# Incorrect: original cause is silently discarded.
143+
except OSError as e:
144+
raise RuntimeError(f"Failed to read model file: {e}")
145+
```
146+
147+
#### Warnings
148+
149+
Use `warnings.warn()` for non-fatal conditions that the caller should know
150+
about. Always pass both `category` and `stacklevel` explicitly:
151+
152+
```python
153+
import warnings
154+
155+
warnings.warn("Message.", UserWarning, stacklevel=2)
156+
```
157+
158+
Recommended categories:
159+
160+
| Situation | Category |
161+
| --- | --- |
162+
| Deprecated API that will be removed in a future version | `DeprecationWarning` |
163+
| Skipped or degraded behavior the caller should review | `UserWarning` |
164+
165+
Use `stacklevel=2` so the warning points at the **caller's** line, not the
166+
internal line that called `warnings.warn()`. Increase `stacklevel` by one for
167+
each additional layer of indirection between the public API and the
168+
`warnings.warn()` call.
169+
170+
Warning messages should be clear, concise, and parseable:
171+
172+
- Write messages as complete sentences ending with a period.
173+
- For deprecations, name the deprecated symbol and its replacement:
174+
175+
```text
176+
<old_symbol> is deprecated; use <new_symbol> instead.
177+
```
178+
179+
- For skipped data or fallback behavior, describe what was skipped and why:
180+
181+
```text
182+
Skipping <item> entry with <reason>: {value!r}
183+
```
184+
185+
Do **not** rely on the default `UserWarning` by omitting `category`.
186+
Always supply `category` explicitly for clarity and greppability.
187+
56188
### Version Control System
57189

58190
- We use [Git](https://git-scm.com/) as our

pythainlp/cli/benchmark.py

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -95,10 +95,11 @@ def __init__(self, name: str, argv: Sequence[str]) -> None:
9595
import yaml
9696

9797
from pythainlp.benchmarks import word_tokenization
98-
except ImportError:
98+
except ImportError as e:
9999
raise ImportError(
100-
"Please install the extra dependencies `benchmarks` to use this command by running `pip install pythainlp[benchmarks]`"
101-
)
100+
"The 'benchmarks' extra dependencies are required for this command."
101+
" Install them with: pip install pythainlp[benchmarks]"
102+
) from e
102103

103104
df_raw = word_tokenization.benchmark(expected, actual)
104105

pythainlp/corpus/core.py

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -484,7 +484,7 @@ def _safe_extract_tar(tar: tarfile.TarFile, path: str) -> None:
484484
tarfile.LinkOutsideDestinationError,
485485
) as e:
486486
# Re-raise as ValueError for consistency with older Python versions
487-
raise ValueError(str(e))
487+
raise ValueError(str(e)) from e
488488
else:
489489
# Manual validation for older Python versions
490490
for member in tar.getmembers():
@@ -900,13 +900,11 @@ def get_hf_hub(repo_id: str, filename: str = "") -> str:
900900
"""
901901
try:
902902
from huggingface_hub import hf_hub_download, snapshot_download
903-
except ModuleNotFoundError:
903+
except ModuleNotFoundError as e:
904904
raise ModuleNotFoundError(
905-
"""
906-
huggingface-hub isn't found!
907-
Please installing the package via 'pip install huggingface-hub'.
908-
"""
909-
)
905+
"huggingface-hub is not installed."
906+
" Install it with: pip install huggingface-hub"
907+
) from e
910908
except Exception as e:
911909
raise RuntimeError(f"An unexpected error occurred: {e}") from e
912910
hf_root = get_full_data_path("hf_models")

pythainlp/parse/attaparse_engine.py

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -12,10 +12,10 @@
1212

1313
try:
1414
from attaparse import depparse, load_model
15-
except ImportError:
15+
except ImportError as e:
1616
raise ImportError(
17-
"Import Error; Install attaparse by pip install attaparse"
18-
)
17+
"attaparse is not installed. Install it with: pip install attaparse"
18+
) from e
1919

2020
if TYPE_CHECKING:
2121
from stanza import Pipeline

pythainlp/parse/esupar_engine.py

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,8 +9,10 @@
99

1010
try:
1111
import esupar
12-
except ImportError:
13-
raise ImportError("Import Error; Install esupar by pip install esupar")
12+
except ImportError as e:
13+
raise ImportError(
14+
"esupar is not installed. Install it with: pip install esupar"
15+
) from e
1416

1517
if TYPE_CHECKING:
1618
from esupar import Model

pythainlp/phayathaibert/core.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -407,8 +407,8 @@ def get_ner(
407407

408408
if pos:
409409
warnings.warn(
410-
"This model doesn't support output \
411-
postag and It doesn't output the postag.",
410+
"This model does not support POS tag output.",
411+
UserWarning,
412412
stacklevel=2,
413413
)
414414

pythainlp/spell/phunspell.py

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -19,10 +19,10 @@
1919

2020
try:
2121
import phunspell
22-
except ImportError:
22+
except ImportError as e:
2323
raise ImportError(
24-
"Import Error; Install phunspell by pip install phunspell"
25-
)
24+
"phunspell is not installed. Install it with: pip install phunspell"
25+
) from e
2626

2727
pspell: "phunspell.Phunspell" = phunspell.Phunspell("th_TH")
2828

pythainlp/spell/symspellpy.py

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -22,10 +22,10 @@
2222

2323
try:
2424
from symspellpy import SymSpell, Verbosity
25-
except ImportError:
25+
except ImportError as e:
2626
raise ImportError(
27-
"Import Error; Install symspellpy by pip install symspellpy"
28-
)
27+
"symspellpy is not installed. Install it with: pip install symspellpy"
28+
) from e
2929

3030
from pythainlp.corpus import get_corpus_path
3131

pythainlp/spell/tltk.py

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,10 +16,10 @@
1616

1717
try:
1818
from tltk.nlp import spell_candidates
19-
except ImportError:
19+
except ImportError as e:
2020
raise ImportError(
21-
"Not found tltk! Please install tltk by pip install tltk"
22-
)
21+
"tltk is not installed. Install it with: pip install tltk"
22+
) from e
2323

2424

2525
def spell(text: str) -> list[str]:

pythainlp/tag/pos_tag.py

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -216,10 +216,10 @@ def pos_tag_transformers(
216216
AutoTokenizer,
217217
TokenClassificationPipeline,
218218
)
219-
except ImportError:
219+
except ImportError as e:
220220
raise ImportError(
221-
"Not found transformers! Please install transformers by pip install transformers"
222-
)
221+
"transformers is not installed. Install it with: pip install transformers"
222+
) from e
223223

224224
if not sentence:
225225
return []

0 commit comments

Comments
 (0)