Skip to content

Commit 85b4348

Browse files
committed
Introduce DiagnosticPipError
This introduces an exception and presentation model, for providing better errors messages. The motivating idea is that the better error messages contain clear wording and provide additional context to users to better understand what is happening. The `DiagnosticPipError` class introduces a structured framework in our exception model, for code authors to write their error messages. The usage explicitly requires passing "context" and a "hint" (which accept None values). This should nudge code authors to explicitly think about what additional information can/should be presented to the user, and to provide relevant hints to them whenever possible. It also makes it straightforward to identify cases where we don't do this, which may serve as clear areas for improvement in the future. The initial implementation is intentionally basic and doesn't do much; however we should be able to introduce better usage of terminal colors and other features (eg: hyperlinks!) to further improve the presentation of these errors. It does improve the presentation style by a bit, even though there are significant presentation-related improvements to be made. Additionally, having a structured framework means that these would be improvements in presentation of *all* the errors that are within this framework -- increasing the benefits of investing in the presentation of these errors.
1 parent 7b3c122 commit 85b4348

File tree

3 files changed

+300
-2
lines changed

3 files changed

+300
-2
lines changed

src/pip/_internal/cli/base_command.py

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
from pip._internal.exceptions import (
2323
BadCommand,
2424
CommandError,
25+
DiagnosticPipError,
2526
InstallationError,
2627
NetworkConnectionError,
2728
PreviousBuildDirError,
@@ -169,6 +170,11 @@ def exc_logging_wrapper(*args: Any) -> int:
169170
logger.debug("Exception information:", exc_info=True)
170171

171172
return PREVIOUS_BUILD_DIR_ERROR
173+
except DiagnosticPipError as exc:
174+
logger.critical(str(exc))
175+
logger.debug("Exception information:", exc_info=True)
176+
177+
return ERROR
172178
except (
173179
InstallationError,
174180
UninstallationError,

src/pip/_internal/exceptions.py

Lines changed: 81 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,23 +1,102 @@
11
"""Exceptions used throughout package"""
22

33
import configparser
4+
import re
45
from itertools import chain, groupby, repeat
5-
from typing import TYPE_CHECKING, Dict, List, Optional, Union
6+
from typing import TYPE_CHECKING, Dict, Iterator, List, Optional, Union
67

78
from pip._vendor.pkg_resources import Distribution
89
from pip._vendor.requests.models import Request, Response
910

1011
if TYPE_CHECKING:
1112
from hashlib import _Hash
13+
from typing import Literal
1214

1315
from pip._internal.metadata import BaseDistribution
1416
from pip._internal.req.req_install import InstallRequirement
1517

1618

19+
#
20+
# Scaffolding
21+
#
22+
def _is_kebab_case(s: str) -> bool:
23+
return re.match(r"^[a-z]+(-[a-z]+)*$", s) is not None
24+
25+
26+
def _prefix_with_indent(prefix: str, s: str, indent: Optional[str] = None) -> str:
27+
if indent is None:
28+
indent = " " * len(prefix)
29+
else:
30+
assert len(indent) == len(prefix)
31+
message = s.replace("\n", "\n" + indent)
32+
return f"{prefix}{message}\n"
33+
34+
1735
class PipError(Exception):
18-
"""Base pip exception"""
36+
"""The base pip error."""
37+
38+
39+
class DiagnosticPipError(PipError):
40+
"""A pip error, that presents diagnostic information to the user.
41+
42+
This contains a bunch of logic, to enable pretty presentation of our error
43+
messages. Each error gets a unique reference. Each error can also include
44+
additional context, a hint and/or a note -- which are presented with the
45+
main erorr message in a consistent style.
46+
"""
47+
48+
reference: str
49+
50+
def __init__(
51+
self,
52+
*,
53+
message: str,
54+
context: Optional[str],
55+
hint_stmt: Optional[str],
56+
attention_stmt: Optional[str] = None,
57+
reference: Optional[str] = None,
58+
kind: 'Literal["error", "warning"]' = "error",
59+
) -> None:
60+
61+
# Ensure a proper reference is provided.
62+
if reference is None:
63+
assert hasattr(self, "reference"), "error reference not provided!"
64+
reference = self.reference
65+
assert _is_kebab_case(reference), "error reference must be kebab-case!"
66+
67+
super().__init__(f"{reference}: {message}")
68+
69+
self.kind = kind
70+
self.message = message
71+
self.context = context
72+
73+
self.reference = reference
74+
self.attention_stmt = attention_stmt
75+
self.hint_stmt = hint_stmt
76+
77+
def __str__(self) -> str:
78+
return "".join(self._string_parts())
79+
80+
def _string_parts(self) -> Iterator[str]:
81+
# Present the main message, with relevant context indented.
82+
yield f"{self.message}\n"
83+
if self.context is not None:
84+
yield f"\n{self.context}\n"
85+
86+
# Space out the note/hint messages.
87+
if self.attention_stmt is not None or self.hint_stmt is not None:
88+
yield "\n"
89+
90+
if self.attention_stmt is not None:
91+
yield _prefix_with_indent("Note: ", self.attention_stmt)
92+
93+
if self.hint_stmt is not None:
94+
yield _prefix_with_indent("Hint: ", self.hint_stmt)
1995

2096

97+
#
98+
# Actual Errors
99+
#
21100
class ConfigurationError(PipError):
22101
"""General exception in configuration"""
23102

tests/unit/test_exceptions.py

Lines changed: 213 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,213 @@
1+
"""Tests the presentation style of exceptions."""
2+
3+
import textwrap
4+
5+
import pytest
6+
7+
from pip._internal.exceptions import DiagnosticPipError
8+
9+
10+
class TestDiagnosticPipErrorCreation:
11+
def test_fails_without_reference(self) -> None:
12+
class DerivedError(DiagnosticPipError):
13+
pass
14+
15+
with pytest.raises(AssertionError) as exc_info:
16+
DerivedError(message="", context=None, hint_stmt=None)
17+
18+
assert str(exc_info.value) == "error reference not provided!"
19+
20+
def test_can_fetch_reference_from_subclass(self) -> None:
21+
class DerivedError(DiagnosticPipError):
22+
reference = "subclass-reference"
23+
24+
obj = DerivedError(message="", context=None, hint_stmt=None)
25+
assert obj.reference == "subclass-reference"
26+
27+
def test_can_fetch_reference_from_arguments(self) -> None:
28+
class DerivedError(DiagnosticPipError):
29+
pass
30+
31+
obj = DerivedError(
32+
message="", context=None, hint_stmt=None, reference="subclass-reference"
33+
)
34+
assert obj.reference == "subclass-reference"
35+
36+
@pytest.mark.parametrize(
37+
"name",
38+
[
39+
"BADNAME",
40+
"BadName",
41+
"bad_name",
42+
"BAD_NAME",
43+
"_bad",
44+
"bad-name-",
45+
"bad--name",
46+
"-bad-name",
47+
"bad-name-due-to-1-number",
48+
],
49+
)
50+
def test_rejects_non_kebab_case_names(self, name: str) -> None:
51+
class DerivedError(DiagnosticPipError):
52+
reference = name
53+
54+
with pytest.raises(AssertionError) as exc_info:
55+
DerivedError(message="", context=None, hint_stmt=None)
56+
57+
assert str(exc_info.value) == "error reference must be kebab-case!"
58+
59+
60+
class TestDiagnosticPipErrorPresentation_ASCII:
61+
def test_complete(self) -> None:
62+
err = DiagnosticPipError(
63+
reference="test-diagnostic",
64+
message="Oh no!\nIt broke. :(",
65+
context="Something went wrong\nvery wrong.",
66+
attention_stmt="You did something wrong, which is what caused this error.",
67+
hint_stmt="Do it better next time, by trying harder.",
68+
)
69+
70+
assert str(err) == textwrap.dedent(
71+
"""\
72+
Oh no!
73+
It broke. :(
74+
75+
Something went wrong
76+
very wrong.
77+
78+
Note: You did something wrong, which is what caused this error.
79+
Hint: Do it better next time, by trying harder.
80+
"""
81+
)
82+
83+
def test_no_context(self) -> None:
84+
err = DiagnosticPipError(
85+
reference="test-diagnostic",
86+
message="Oh no!\nIt broke. :(",
87+
context=None,
88+
attention_stmt="You did something wrong, which is what caused this error.",
89+
hint_stmt="Do it better next time, by trying harder.",
90+
)
91+
92+
assert str(err) == textwrap.dedent(
93+
"""\
94+
Oh no!
95+
It broke. :(
96+
97+
Note: You did something wrong, which is what caused this error.
98+
Hint: Do it better next time, by trying harder.
99+
"""
100+
)
101+
102+
def test_no_note(self) -> None:
103+
err = DiagnosticPipError(
104+
reference="test-diagnostic",
105+
message="Oh no!\nIt broke. :(",
106+
context="Something went wrong\nvery wrong.",
107+
attention_stmt=None,
108+
hint_stmt="Do it better next time, by trying harder.",
109+
)
110+
111+
assert str(err) == textwrap.dedent(
112+
"""\
113+
Oh no!
114+
It broke. :(
115+
116+
Something went wrong
117+
very wrong.
118+
119+
Hint: Do it better next time, by trying harder.
120+
"""
121+
)
122+
123+
def test_no_hint(self) -> None:
124+
err = DiagnosticPipError(
125+
reference="test-diagnostic",
126+
message="Oh no!\nIt broke. :(",
127+
context="Something went wrong\nvery wrong.",
128+
attention_stmt="You did something wrong, which is what caused this error.",
129+
hint_stmt=None,
130+
)
131+
132+
assert str(err) == textwrap.dedent(
133+
"""\
134+
Oh no!
135+
It broke. :(
136+
137+
Something went wrong
138+
very wrong.
139+
140+
Note: You did something wrong, which is what caused this error.
141+
"""
142+
)
143+
144+
def test_no_context_no_hint(self) -> None:
145+
err = DiagnosticPipError(
146+
reference="test-diagnostic",
147+
message="Oh no!\nIt broke. :(",
148+
context=None,
149+
attention_stmt="You did something wrong, which is what caused this error.",
150+
hint_stmt=None,
151+
)
152+
153+
assert str(err) == textwrap.dedent(
154+
"""\
155+
Oh no!
156+
It broke. :(
157+
158+
Note: You did something wrong, which is what caused this error.
159+
"""
160+
)
161+
162+
def test_no_context_no_note(self) -> None:
163+
err = DiagnosticPipError(
164+
reference="test-diagnostic",
165+
message="Oh no!\nIt broke. :(",
166+
context=None,
167+
attention_stmt=None,
168+
hint_stmt="Do it better next time, by trying harder.",
169+
)
170+
171+
assert str(err) == textwrap.dedent(
172+
"""\
173+
Oh no!
174+
It broke. :(
175+
176+
Hint: Do it better next time, by trying harder.
177+
"""
178+
)
179+
180+
def test_no_hint_no_note(self) -> None:
181+
err = DiagnosticPipError(
182+
reference="test-diagnostic",
183+
message="Oh no!\nIt broke. :(",
184+
context="Something went wrong\nvery wrong.",
185+
attention_stmt=None,
186+
hint_stmt=None,
187+
)
188+
189+
assert str(err) == textwrap.dedent(
190+
"""\
191+
Oh no!
192+
It broke. :(
193+
194+
Something went wrong
195+
very wrong.
196+
"""
197+
)
198+
199+
def test_no_hint_no_note_no_context(self) -> None:
200+
err = DiagnosticPipError(
201+
reference="test-diagnostic",
202+
message="Oh no!\nIt broke. :(",
203+
context=None,
204+
hint_stmt=None,
205+
attention_stmt=None,
206+
)
207+
208+
assert str(err) == textwrap.dedent(
209+
"""\
210+
Oh no!
211+
It broke. :(
212+
"""
213+
)

0 commit comments

Comments
 (0)