-
Notifications
You must be signed in to change notification settings - Fork 21
Fix handling for unknown config variable in pyodide config get
#200
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
base: main
Are you sure you want to change the base?
Conversation
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) |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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()
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No description provided.