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

config/themes: Convert pygments styles to urwid compatible styles. #1452

Merged
merged 2 commits into from
Jan 8, 2024

Conversation

jrijul1201
Copy link
Collaborator

@jrijul1201 jrijul1201 commented Dec 19, 2023

What does this PR do, and why?

Pygments 2.16.0 introduced a style to support a combination of bold and italic styling in pygments/pygments#2444. Both of our gruvbox themes and the light native theme gain a 'bold strong' style via pygments as a result, which urwid fails to parse and blocks the application from loading.

This work would represent a clean fix for #1431, which was temporarily fixed by pinning pygments at ~=2.15.1 in #1433. Pygments can be unpinned after this fix.

Add method translate_styles that manually converts the pygments bold italic style into the urwid-compatible bold, italics.

In requirements, unpin pygments from ~=2.15.1 to >=2.14.0,<2.18.0.

External discussion & connections

  • Discussed in #zulip-terminal in Translate pygments styles into urwid #T1434 #T1452
  • Fully fixes #
  • Partially fixes issue Improve translation of pygments styles into urwid #1434
  • Builds upon previous unmerged work in PR #
  • Is a follow-up to work in PR #
  • Requires merge of PR #
  • Merge will enable work on #

How did you test this?

  • Manually - Behavioral changes
  • Manually - Visual changes
  • Adapting existing automated tests
  • Adding automated tests for new behavior (or missing tests)
  • Existing automated tests should already cover this (only a refactor of tested code)

Self-review checklist for each commit

  • It is a minimal coherent idea
  • It has a commit summary following the documented style (title & body)
  • It has a commit summary describing the motivation and reasoning for the change
  • It individually passes linting and tests
  • It contains test additions for any new behavior
  • It flows clearly from a previous branch commit, and/or prepares for the next commit

@zulipbot zulipbot added the size: L [Automatic label added by zulipbot] label Dec 19, 2023
@jrijul1201 jrijul1201 marked this pull request as ready for review December 19, 2023 08:12
Copy link
Collaborator

@neiljp neiljp left a comment

Choose a reason for hiding this comment

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

@jrijul1201 Thanks for working on this! 👍

I tried upgrading pygments, and I at least don't have an issue with a crash. I would assume it's making a correct translation, and would suggest a before and after comparison - except that the pygments styles did change, of course!

Most of my comments are minor, with the exception of ensuring the tests cover what you expect.

You're welcome to add a commit to upgrade pygments following the standard form we've used in the past.

zulipterminal/config/themes.py Outdated Show resolved Hide resolved
zulipterminal/config/themes.py Outdated Show resolved Hide resolved
tests/config/test_themes.py Outdated Show resolved Hide resolved
tests/config/test_themes.py Outdated Show resolved Hide resolved
Comment on lines 466 to 467
for style in expected_styles:
assert style in pygments_styles
Copy link
Collaborator

Choose a reason for hiding this comment

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

This doesn't test what you might think; I can alter the input or output styles and the tests all pass.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed, Thanks a lot for the thorough review!

@neiljp neiljp added bug Something isn't working area: colors/styles/themes labels Dec 20, 2023
@jrijul1201 jrijul1201 force-pushed the 1434-solving branch 2 times, most recently from 16a94be to 24ee3f2 Compare December 20, 2023 15:48
Copy link
Collaborator

@neiljp neiljp left a comment

Choose a reason for hiding this comment

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

@jrijul1201 Good changes, with a few suggestions - though give those tests a reread/explore :)

tests/config/test_themes.py Show resolved Hide resolved
tests/config/test_themes.py Outdated Show resolved Hide resolved
tests/config/test_themes.py Outdated Show resolved Hide resolved
zulipterminal/config/themes.py Outdated Show resolved Hide resolved
zulipterminal/config/themes.py Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
zulipterminal/config/themes.py Show resolved Hide resolved
zulipterminal/config/themes.py Outdated Show resolved Hide resolved
@jrijul1201 jrijul1201 force-pushed the 1434-solving branch 3 times, most recently from 9e3b6b1 to 0ac17c0 Compare December 22, 2023 09:55
jrijul1201 added a commit to jrijul1201/zulip-terminal that referenced this pull request Dec 22, 2023
The previous commit in zulip#1452 fixes the issue zulip#1431 that was temporarily
fixed by pinning Pygments at ~=2.15.1.

Combined with <2.18.0, since that unreleased version depends on
Python3.8 and could make ZT uninstallable on older pythons.

Fixes part of zulip#1434.
Copy link
Collaborator

@neiljp neiljp left a comment

Choose a reason for hiding this comment

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

@jrijul1201 Thanks for the update 👍

Your latest push was great, and I've just pushed a very slightly updated version here, which I'll merge shortly :)

The inline comments were some minor feedback on the tests to consider in the future - I've addressed them in my push.

I also updated the commit text for the first commit

  • bold strong seemed incorrect vs bold italic?
  • there is no longer a 'translate_styles' function; instead I clarified it minimally covers certain translations, and not all of the possibilities from pygments (which may change in future, of course)

For details of any other changes, I'd suggest you use git range-diff or an equivalent tool.

Comment on lines 432 to 434
"token1": "bold italic",
"token2": "italic bold",
"token3": "italic #abc",
Copy link
Collaborator

Choose a reason for hiding this comment

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

This case tests multiple aspects at once - that's fine, depending on how small we wish each test/case to be.

However, if these were separate tests, we might give ids to the tokens to indicate the reason for including them. For here, it therefore could be useful to add a few minimal comments to indicate eg. lack of ordering-dependence, and working with colors.

Comment on lines 458 to 462
def test_generate_urwid_compatible_pygments_styles(
pygments_styles: Dict[_TokenType, str],
expected_styles: Dict[_TokenType, str],
style_translations: Dict[str, str],
) -> None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's great to have the annotations here 👍

This works fine in the test itself, however note that mypy/pytest doesn't validate that the parametrize values match these types, only that the function parameters work with the body.

That doesn't invalidate the test, as long as we acknowledge that we're using placeholder values, with eg. a comment regarding the "token" values (str) not being of type _TokenType.

To improve upon this, one could imagine testing with actual pygments styles, but that's not necessary for this PR.

generated_styles = generate_urwid_compatible_pygments_styles(
pygments_styles, style_translations
)
assert sorted(expected_styles.items()) == sorted(generated_styles.items())
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks good 👍

The test values are good to cover many cases, but we can also assert the keys are unchanged, as a quick check before we look at the values. That allows a test to fail with a more significant assertion first.

Pygments 2.16.0 introduced a style to support a combination of bold and
italic styling in pygments/pygments#2444. Both of our gruvbox themes and
the light native theme gain a 'bold italic' style via pygments as a
result, which urwid fails to parse and blocks the application from
loading.

This work would represent a clean fix for zulip#1431, which was temporarily
fixed by pinning pygments at ~=2.15.1 in zulip#1433.

This minimally handles spaces and the shortened `italic` in pygments
styles, sufficient to resolve the current style issue. Note that other
pygments styles in themes that we do not yet use may need additional
translation or adjustment.

Tests added.

Fixes part of zulip#1434.
The previous commit fixes the issue zulip#1431 that was temporarily
fixed by pinning pygments at ~=2.15.1 in zulip#1433.

Subsequent versions of pygments have since been released, but an upper
limit of 2.18.0 is included, since that unreleased version depends on
Python3.8 and could make ZT uninstallable on older pythons.

Fixes part of zulip#1434.
@neiljp neiljp merged commit 646a04e into zulip:main Jan 8, 2024
20 checks passed
@neiljp neiljp added this to the Next Release milestone Jan 8, 2024
@neiljp
Copy link
Collaborator

neiljp commented Jan 8, 2024

@jrijul1201 Thanks for your work on this 👍 I rebased over the CI workaround in #1458, so merged this a little later than anticipated, but it's in now :) 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: colors/styles/themes bug Something isn't working size: L [Automatic label added by zulipbot]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants