jrnl icon indicating copy to clipboard operation
jrnl copied to clipboard

Drop/replace ansiwrap dependency

Open wren opened this issue 3 years ago • 6 comments

Bug Report

Environment

  • Local env (running pytest)

Current Behavior

When running pytest, I see a deprecation notice for ansiwrap (`import imp`). When looking into ansiwrap, I find that they already have an issue filed (jonathaneunice/ansiwrap#17), but no response or updates from the project in more than 6 months. The project itself also hasn't been updated in ages, so we should probably look for a replacement.

Expected Behavior

We don't get deprecation warnings when running our test suite.

Repro Steps

  1. Run pytest
  2. See warning
================================== warnings summary ===================================
../../../Library/Caches/pypoetry/virtualenvs/jrnl-GFqVlfgP-py3.9/lib/python3.9/site-packages/ansiwrap/core.py:6
  /Users/jonathan/Library/Caches/pypoetry/virtualenvs/jrnl-GFqVlfgP-py3.9/lib/python3.9/site-packages/ansiwrap/core.py:6: DeprecationWarning: the imp module is deprecated in favour of importlib; see the module's documentation for alternative uses
    import imp

-- Docs: https://docs.pytest.org/en/stable/warnings.html
============================ 77 passed, 1 warning in 0.84s ============================

wren avatar Feb 14 '21 00:02 wren

I've heard of rich being used for terminal output. From a quick glance at their documentation, it seems to fit the bill.

sriniv27 avatar Feb 20 '21 23:02 sriniv27

@sriniv27 Rich looks really good at first glance. We'll take a closer look and hopefully replace both ansiwrap and colorama with this. Thank you for the suggestion!

wren avatar Feb 27 '21 18:02 wren

I'm pretty sure rich relies on colorama under the hood. I have yet to come across another library (other than colorama) that provides color on Windows and Linux cleanly.

MinchinWeb avatar May 01 '21 20:05 MinchinWeb

I think you're right, @MinchinWeb. It looks like they use colorama under the hood when an old windows terminal is detected (since the new windows terminal supports more colors). So, using rich would let us use fancy new features without dropping legacy support for older windows prompts (which is the best of both worlds in my eyes).

wren avatar May 02 '21 00:05 wren

I'm planning on updating the jrnl port in MacPorts. ansiwrap has an issue with their textwrap3 dependency that is preventing it from being a simple update on our side.

This would normally be an easy fix to sort out upstream, but as @wren mentions, ansiwarp hasn't been maintained for ages now. I'm therefore all in favour of switching to an alternative, such as rich.

It goes without saying, but thank you all for maintaining this amazing project!

harens avatar Jun 05 '21 14:06 harens

@harens Thank you! I'm always happy to hear that people are enjoying jrnl. I'd like to take a moment to call out that while I tend to be a bit more public, that I absolutely couldn't do it without @micahellison doing so much work that isn't as publicly visible (we both spend our full Saturdays working on jrnl). So, thank you, Micah!

I'm also happy to hear that someone wants to help with a package manager. We've been talking about the various package mangers that we would like to support (#1171), and it's honestly very daunting since each one can be so different.

As for timing to remove this ansiwrap dependency: we're currently focused on updating the test suite (#1192) and revamping our plugin system (#1239, #1006), but the very next release after that is going to focus entirely on bug fixes and maintenance. So, we can't quite get to this right now, but it should be pretty soon hopefully.

In the meantime, if anyone feels up to taking this on, we would happily review PRs to fix this sooner.

wren avatar Jun 06 '21 03:06 wren

I'm feeling less confident about using rich to replace ansiwrap. There's no 1:1 mapping of the ansiwrap methods we need (fill and strip_color) to rich, but also rich's documented behavior is centered around display rather than string manipulation. We could reach into rich's classes and use their functionality (specifically, the Console, Text, and Lines classes, and possibly AnsiDecoder), which are well-documented in the code, but it makes our code base more vulnerable to changes in rich, and it's not really an intended use of rich.

In other words, I think it can be done. I'm just not sure if it should. Maybe there's a better way.

micahellison avatar Feb 23 '23 16:02 micahellison

Also, in Python 3.11, the deprecation notice triggered by ansiwrap states that the deprecated function will be removed in Python 3.12, so this issue needs to be fixed before November 2023.

micahellison avatar Feb 23 '23 17:02 micahellison