Skip to content
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

[bugfix] solve crash when using inspect() on the "pyparsing" package #2294

Merged
merged 3 commits into from May 27, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Expand Up @@ -10,6 +10,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
### Fixed

- Allow exceptions that are raised while a Live is rendered to be displayed and/or processed https://github.com/Textualize/rich/pull/2305
- Fix crashes that can happen with `inspect` when docstrings contain some special control codes https://github.com/Textualize/rich/pull/2294

## [12.4.4] - 2022-05-24

Expand Down
3 changes: 3 additions & 0 deletions pyproject.toml
Expand Up @@ -61,3 +61,6 @@ enable_error_code = ["ignore-without-code", "redundant-expr", "truthy-bool"]
[[tool.mypy.overrides]]
module = ["pygments.*", "IPython.*", "commonmark.*", "ipywidgets.*"]
ignore_missing_imports = true

[tool.pytest.ini_options]
testpaths = ["tests"]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

while working on this I noticed that creating a Python script with a name starting ending with test_ anywhere in the project's repo was collected by Pytest: I think we'd better tell Pytest to stick with the contents of our tests/ folder - which should also make the tests collection faster as a side effect

43 changes: 27 additions & 16 deletions rich/_inspect.py
Expand Up @@ -5,6 +5,7 @@
from typing import Any, Iterable, Optional, Tuple

from .console import Group, RenderableType
from .control import escape_control_codes
from .highlighter import ReprHighlighter
from .jupyter import JupyterMixin
from .panel import Panel
Expand All @@ -19,12 +20,6 @@ def _first_paragraph(doc: str) -> str:
return paragraph


def _reformat_doc(doc: str) -> str:
"""Reformat docstring."""
doc = cleandoc(doc).strip()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed that we were ending up calling cleandoc twice on our extracted docstrings - to remedy this I inlined this operation in a single _get_formatted_doc method, later on this same module

return doc


class Inspect(JupyterMixin):
"""A renderable to inspect any Python Object.

Expand Down Expand Up @@ -161,11 +156,9 @@ def safe_getattr(attr_name: str) -> Tuple[Any, Any]:
yield ""

if self.docs:
_doc = getdoc(obj)
_doc = self._get_formatted_doc(obj)
if _doc is not None:
if not self.help:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The process of calling cleandoc() on the doctring's content and getting only its first paragraph if self.help is not True is now factorised in a single method, rather than done twice in this class

_doc = _first_paragraph(_doc)
doc_text = Text(_reformat_doc(_doc), style="inspect.help")
doc_text = Text(_doc, style="inspect.help")
doc_text = highlighter(doc_text)
yield doc_text
yield ""
Expand Down Expand Up @@ -200,13 +193,10 @@ def safe_getattr(attr_name: str) -> Tuple[Any, Any]:
add_row(key_text, Pretty(value, highlighter=highlighter))
else:
if self.docs:
docs = getdoc(value)
docs = self._get_formatted_doc(value)
if docs is not None:
_doc = _reformat_doc(str(docs))
if not self.help:
_doc = _first_paragraph(_doc)
_signature_text.append("\n" if "\n" in _doc else " ")
doc = highlighter(_doc)
_signature_text.append("\n" if "\n" in docs else " ")
doc = highlighter(docs)
doc.stylize("inspect.doc")
_signature_text.append(doc)

Expand All @@ -220,3 +210,24 @@ def safe_getattr(attr_name: str) -> Tuple[Any, Any]:
f"[b cyan]{not_shown_count}[/][i] attribute(s) not shown.[/i] "
f"Run [b][magenta]inspect[/]([not b]inspect[/])[/b] for options."
)

def _get_formatted_doc(self, object_: Any) -> Optional[str]:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you mind adding a docstring.

"""
Extract the docstring of an object, process it and returns it.
The processing consists in cleaning up the doctring's indentation,
taking only its 1st paragraph if `self.help` is not True,
and escape its control codes.

Args:
object_ (Any): the object to get the docstring from.

Returns:
Optional[str]: the processed docstring, or None if no docstring was found.
"""
docs = getdoc(object_)
if docs is None:
return None
docs = cleandoc(docs).strip()
if not self.help:
docs = _first_paragraph(docs)
return escape_control_codes(docs)
38 changes: 35 additions & 3 deletions rich/control.py
@@ -1,19 +1,35 @@
import sys
import time
from typing import TYPE_CHECKING, Callable, Dict, Iterable, List, Union

if sys.version_info >= (3, 8):
from typing import Final
else:
from typing_extensions import Final # pragma: no cover

from .segment import ControlCode, ControlType, Segment

if TYPE_CHECKING:
from .console import Console, ConsoleOptions, RenderResult

STRIP_CONTROL_CODES = [
STRIP_CONTROL_CODES: Final = [
7, # Bell
8, # Backspace
11, # Vertical tab
12, # Form feed
13, # Carriage return
]
_CONTROL_TRANSLATE = {_codepoint: None for _codepoint in STRIP_CONTROL_CODES}
_CONTROL_STRIP_TRANSLATE: Final = {
willmcgugan marked this conversation as resolved.
Show resolved Hide resolved
_codepoint: None for _codepoint in STRIP_CONTROL_CODES
}

CONTROL_ESCAPE: Final = {
7: "\\a",
8: "\\b",
11: "\\v",
12: "\\f",
13: "\\r",
}

CONTROL_CODES_FORMAT: Dict[int, Callable[..., str]] = {
ControlType.BELL: lambda: "\x07",
Expand Down Expand Up @@ -169,7 +185,7 @@ def __rich_console__(


def strip_control_codes(
text: str, _translate_table: Dict[int, None] = _CONTROL_TRANSLATE
text: str, _translate_table: Dict[int, None] = _CONTROL_STRIP_TRANSLATE
) -> str:
"""Remove control codes from text.

Expand All @@ -182,6 +198,22 @@ def strip_control_codes(
return text.translate(_translate_table)


def escape_control_codes(
text: str,
_translate_table: Dict[int, str] = CONTROL_ESCAPE,
) -> str:
"""Replace control codes with their "escaped" equivalent in the given text.
(e.g. "\b" becomes "\\b")

Args:
text (str): A string possibly containing control codes.

Returns:
str: String with control codes replaced with their escaped version.
"""
return text.translate(_translate_table)


if __name__ == "__main__": # pragma: no cover
from rich.console import Console

Expand Down
17 changes: 9 additions & 8 deletions rich/text.py
Expand Up @@ -2,7 +2,6 @@
from functools import partial, reduce
from math import gcd
from operator import itemgetter
from rich.emoji import EmojiVariant
from typing import (
TYPE_CHECKING,
Any,
Expand Down Expand Up @@ -141,15 +140,16 @@ def __init__(
tab_size: Optional[int] = 8,
spans: Optional[List[Span]] = None,
) -> None:
self._text = [strip_control_codes(text)]
sanitized_text = strip_control_codes(text)
self._text = [sanitized_text]
self.style = style
self.justify: Optional["JustifyMethod"] = justify
self.overflow: Optional["OverflowMethod"] = overflow
self.no_wrap = no_wrap
self.end = end
self.tab_size = tab_size
self._spans: List[Span] = spans or []
self._length: int = len(text)
self._length: int = len(sanitized_text)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's the bugfix itself! 🙂 i.e. we should use the length of the sanitised text, rather than the one of the text we received as an argument


def __len__(self) -> int:
return self._length
Expand Down Expand Up @@ -394,9 +394,10 @@ def plain(self) -> str:
def plain(self, new_text: str) -> None:
"""Set the text to a new value."""
if new_text != self.plain:
self._text[:] = [new_text]
sanitized_text = strip_control_codes(new_text)
self._text[:] = [sanitized_text]
old_length = self._length
self._length = len(new_text)
self._length = len(sanitized_text)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same bug here 🙂

if old_length > self._length:
self._trim_spans()

Expand Down Expand Up @@ -906,10 +907,10 @@ def append(

if len(text):
if isinstance(text, str):
text = strip_control_codes(text)
self._text.append(text)
sanitized_text = strip_control_codes(text)
self._text.append(sanitized_text)
offset = len(self)
text_length = len(text)
text_length = len(sanitized_text)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...and here

if style is not None:
self._spans.append(Span(offset, offset + text_length, style))
self._length += text_length
Expand Down
8 changes: 7 additions & 1 deletion tests/test_control.py
@@ -1,4 +1,4 @@
from rich.control import Control, strip_control_codes
from rich.control import Control, escape_control_codes, strip_control_codes
from rich.segment import ControlType, Segment


Expand All @@ -13,6 +13,12 @@ def test_strip_control_codes():
assert strip_control_codes("Fear is the mind killer") == "Fear is the mind killer"


def test_escape_control_codes():
assert escape_control_codes("") == ""
assert escape_control_codes("foo\rbar") == "foo\\rbar"
assert escape_control_codes("Fear is the mind killer") == "Fear is the mind killer"


def test_control_move_to():
control = Control.move_to(5, 10)
print(control.segment)
Expand Down
38 changes: 38 additions & 0 deletions tests/test_inspect.py
Expand Up @@ -299,3 +299,41 @@ class Thing:
"╰──────────────────────────────────────────╯\n"
)
assert render(module, methods=True) == expected


@pytest.mark.parametrize(
"special_character,expected_replacement",
(
("\a", "\\a"),
("\b", "\\b"),
("\f", "\\f"),
("\r", "\\r"),
("\v", "\\v"),
),
)
def test_can_handle_special_characters_in_docstrings(
special_character: str, expected_replacement: str
):
class Something:
class Thing:
pass

Something.Thing.__doc__ = f"""
Multiline docstring
with {special_character} should be handled
"""

expected = """\
╭─ <class 'tests.test_inspect.test_can_handle_sp─╮
│ class test_can_handle_special_characters_in_do │
│ cstrings.<locals>.Something(): │
│ │
│ Thing = class Thing(): │
│ Multiline docstring │
│ with %s should be handled │
╰────────────────────────────────────────────────╯
""" % (
expected_replacement
)

assert render(Something, methods=True) == expected
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to make sure that we don't have regressions on this bugfix, before trying to solve it I started by writing a test that reproduce the issue : this test was crashing before the fix, and passes now 🎈