Skip to content

Conversation

hoodmane
Copy link
Member

@hoodmane hoodmane commented May 8, 2025

No description provided.

typer.echo(f"Config variable {config_var} not found.")
typer.Exit(1)
print(f"Config variable {config_var} not found.", file=sys.stderr)
raise typer.Exit(1)
Copy link
Member

Choose a reason for hiding this comment

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

👍

if config_var not in configs:
typer.echo(f"Config variable {config_var} not found.")
typer.Exit(1)
print(f"Config variable {config_var} not found.", file=sys.stderr)
Copy link
Member

Choose a reason for hiding this comment

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

What's the purpose of changing typo.echo to print? If the goal is to avoid using typer APIs we might change it everywhere.

Copy link
Member Author

@hoodmane hoodmane May 18, 2025

Choose a reason for hiding this comment

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

The point is to print to stderr. I was unable to figure out how to do that with typer.echo()

Copy link
Member

Choose a reason for hiding this comment

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

The docs at https://typer.tiangolo.com/tutorial/printing/?h=stderr#printing-to-standard-error say that we need to use stderr=True, but it has to be done at the rich.console.Console() level. Maybe we should set that somewhere?

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried that and didn't fix it in ~10 minutes or so of messing around. I think it matters much more that the print goes to stderr then that it goes through rich. The same looking text shows up on the console at the end either way.

Copy link
Member

@ryanking13 ryanking13 left a comment

Choose a reason for hiding this comment

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

It looks like some tests need to be updated to check result.stderr. Otherwise looks good to me.

Copy link
Member

@agriyakhetarpal agriyakhetarpal left a comment

Choose a reason for hiding this comment

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

Thanks, @hoodmane! I opened #204 to talk about Typer-related issues, so I think this should be good for now.

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.

3 participants