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

remove rich #5033

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

remove rich #5033

wants to merge 12 commits into from

Conversation

adhami3310
Copy link
Member

there are a few remaining [bold]

Copy link

codspeed-hq bot commented Mar 26, 2025

CodSpeed Performance Report

Merging #5033 will not alter performance

Comparing karl-marx (34bb97f) with main (07e056c)

Summary

✅ 12 untouched benchmarks

@staticmethod
def _moveup(lines: int):
for _ in range(lines):
sys.stdout.write("\x1b[A")
Copy link
Collaborator

Choose a reason for hiding this comment

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

should this kind of stuff go to stderr? or maybe take a stream field so its easier to change later

Copy link
Collaborator

Choose a reason for hiding this comment

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

does windows even support these ANSI codes reliably?

rich was at least giving us a cross platform abstraction

Copy link
Member Author

Choose a reason for hiding this comment

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

it's not per OS, more like, per terminal

@@ -113,7 +117,9 @@ def run_process_and_launch_url(
url = urljoin(url, get_config().frontend_path)

console.print(
f"App running at: [bold green]{url}[/bold green]{' (Frontend-only mode)' if not backend_present else ''}"
"App running at: "
+ colored(url, "green", attrs=("bold",))
Copy link
Collaborator

Choose a reason for hiding this comment

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

underline on the link would be nice

msg = colored(msg, color, attrs=["bold"] if bold else [])

if IS_REPRENTER_ACTIVE:
print("\n" + msg, flush=True, **kwargs) # noqa: T201
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 seem like quite the right logic. it causes a bunch of log lines interleaved with status messages. in rich the printed lines seem to go above the status line without leaving artifacts.

Copy link
Member Author

Choose a reason for hiding this comment

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

this doesn't seem like quite the right logic. it causes a bunch of log lines interleaved with status messages. in rich the printed lines seem to go above the status line without leaving artifacts.

this is actually not my experience, in mine i get double progress bars whenever i hit a deprecation warning or such, with rich

@masenf
Copy link
Collaborator

masenf commented Mar 27, 2025

overall i'm a little hesitant on this change still.

image

it needs more testing on different platforms and with different log levels

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants