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

Use rich instead of ansiwrap to wrap text #1693

Merged

Conversation

micahellison
Copy link
Member

@micahellison micahellison commented Feb 27, 2023

Fixes #1191. This PR removes ansiwrap and uses rich instead. It only affects "pretty printing" -- the default display format that jrnl uses.

⚠ This PR modifies the line wrapping behavior slightly: the old method removed trailing spaces, whereas this new method includes them. We don't consider this a breaking change because this format is meant to be read by humans instead of machines.

Checklist

  • I have read the contributing doc.
  • I have included a link to the relevant issue number.
  • I have checked to ensure there aren't other open pull requests
    for the same issue.
  • I have written new tests for these changes, as needed.

@micahellison
Copy link
Member Author

micahellison commented Mar 16, 2023

I am not sure why this last test is failing:

tests/bdd/test_features.py::test_printing_a_journal_that_has_multiline_entries_with_tags

image

Adding a space to the end of the second line makes it pass. However, when I run the test with manual jrnl commands, I don't see that extra space.

The original test result may seem a bit odd, too: the last word is on its own line. Both the original ansiwrap and the new rich wrappers are giving it its own line because the source data is actually already wrapped, and this is the only line in the source data that is at least as long as the linewrap value (80 characters). In fact, it's exactly the length of the linewrap value. Still, I have no explanation as to why this space isn't being preserved in my manual tests. It would be nice to know what's going on instead of just changing the test to make it pass.

@micahellison micahellison mentioned this pull request Jul 15, 2023
4 tasks
poetry.lock retrieved from develop then re-locked with poetry lock --no-update
@micahellison
Copy link
Member Author

We found that the issue is that rich doesn't remove trailing spaces when it wraps, whereas ansiwrap did. We're staying with rich's default behavior and I've made a note about this in the PR description.

@micahellison micahellison marked this pull request as ready for review July 29, 2023 20:53
@micahellison micahellison changed the title [WIP] Use rich instead of ansiwrap to wrap text Use rich instead of ansiwrap to wrap text Jul 29, 2023
@wren wren added the dependencies Pull requests that update a dependency file label Jul 29, 2023
Copy link
Member

@wren wren left a comment

Choose a reason for hiding this comment

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

🫡

@micahellison micahellison merged commit cb69bb4 into jrnl-org:develop Jul 29, 2023
8 checks passed
MinchinWeb added a commit to MinchinWeb/minchin.jrnl that referenced this pull request Sep 22, 2023
Ansiwrap hasn't been updated in several years, and looks like it will break with Python 3.12. Rich bring quite a few sub-dependencies, but is more powerful than it is being used here and actively developed.
c.f. jrnl-org/jrnl#1693
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Drop/replace ansiwrap dependency
2 participants