search

bloomberg / memray

Memray is a memory profiler for Python
https://bloomberg.github.io/memray/
Apache License 2.0
13.17k stars 392 forks source link

Handle changes in Textual 0.48 #543

Closed godlygeek closed 7 months ago

godlygeek commented 7 months ago

Textual 0.48 makes several changes to the TextArea that we need to adapt to

Even with the SVG, outside of the "show differences" mode I can't see the difference between the old: image and the new: image

godlygeek commented 7 months ago

Oof, there's more that needs changing here it seems.

codecov-commenter commented 7 months ago

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (41248ed) 92.55% compared to head (88fbf58) 92.89%. Report is 1 commits behind head on main.

Files Patch % Lines
tests/conftest.py 75.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #543 +/- ## ========================================== + Coverage 92.55% 92.89% +0.34% ========================================== Files 91 91 Lines 11304 11109 -195 Branches 1581 2022 +441 ========================================== - Hits 10462 10320 -142 + Misses 837 789 -48 + Partials 5 0 -5 ``` | [Flag](https://app.codecov.io/gh/bloomberg/memray/pull/543/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bloomberg) | Coverage Δ | | |---|---|---| | [cpp](https://app.codecov.io/gh/bloomberg/memray/pull/543/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bloomberg) | `92.89% <90.00%> (+6.95%)` | :arrow_up: | | [python_and_cython](https://app.codecov.io/gh/bloomberg/memray/pull/543/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bloomberg) | `92.89% <90.00%> (-2.83%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bloomberg#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

godlygeek commented 7 months ago

... Huh. The 3 snapshots I needed to regenerate to get them to pass locally passed on Alpine, Ubuntu 22.04, and macOS, but failed in the manylinux and musllinux containers. What the heck? ☚ī¸

godlygeek commented 7 months ago

Ah - those jobs are testing with CPython 3.7, which Textual dropped support for in 0.44...

OK, I think we need to skip these 3 snapshot tests for Python 3.7, then.

darrenburns commented 7 months ago

Hey 👋

Textual snapshot tests will fail if there is a mismatch in the escape codes Textual writes to the terminal. However, different escape sequences can result in the same visual output. A failing snapshot test does not always mean there is a visual difference, it's more of a nudge to double check things, because different escape sequences have been produced.

In the case of the TextArea, we modified how it renders, which will result in some failing tests.

As for the artefacts you see in the diff, I think that's just down to how the browser is rendering the SVG. We actually overlay the two SVGs and the diff is done via CSS, so I think it's just a rendering anomaly at the browser level 😄

For the TextArea changes, if you want to retain the exact same behaviour as before you can simply substitute calls to the TextArea constructor with TextArea.code_editor, which is a convenience constructor we added which retains the old defaults (wrapping disabled, syntax highlighting enabled, etc.). If it's not your intention to retain the old behaviour entirely, disregard this paragraph!

godlygeek commented 7 months ago

different escape sequences have been produced.

In the case of the TextArea, we modified how it renders, which will result in some failing tests.

Fair enough, that makes sense.

As for the artefacts you see in the diff, I think that's just down to how the browser is rendering the SVG. We actually overlay the two SVGs and the diff is done via CSS, so I think it's just a rendering anomaly at the browser level 😄

Sounds plausible. I was thinking some characters might have been aligned a few pixels off in the SVG from where they were in a previous version. Either way, I agree that it doesn't seem like anything to worry about.

What I'm much more curious about is why switching from DEFAULT_CSS to CSS_PATH fixed some tests that were failing! I know there's some subtle differences between how DEFAULT_CSS is applied versus how CSS in a file is applied. I know DEFAULT_CSS is scoped to the widget and its children by default while CSS_PATH isn't... But I don't think that accounts for the differences, since the widget with the DEFAULT_CSS was the App, and so every other widget is one of its descendants...

For the TextArea changes, if you want to retain the exact same behaviour as before you can simply substitute calls to the TextArea constructor with TextArea.code_editor, which is a convenience constructor we added which retains the old defaults

That was actually a bit uglier to do, since I don't want to drop support for Textual < 0.48 just yet: we'd need to special case on the existence or non-existence of that attribute. I started off writing getattr(TextArea, "code_editor", TextArea)(...) but that seemed arcane, and it seemed clearer just to set soft_wrap = False directly (knowing that attribute would just be ignored in old versions).

the old defaults (wrapping disabled, syntax highlighting enabled, etc.)

I didn't notice that there were more differences between the new defaults and the ones set by code_editor than just soft wrapping, but it turns out we're getting them all right anyway. We're already setting a theme and disabling line numbers and disabling focusing entirely (via can_focus = False), and that seems to be the sum total of the differences.