Remove rich library from fastmigrate #24
Conversation
|
Great. This will resolve #2 . I'll review today. |
jph00
left a comment
jph00
left a comment
There was a problem hiding this comment.
A few minor suggestions (some of them not directly related to the PR, since I haven't seen this code before.)
|
@algal do you have a chance to look thru this and update/merge it? |
|
@jph00 will handle today with other fastmigrate release work. |
|
@jph00 I'm presuming this is okay and doesn't strictly need a re-review from you, since the last commits address all your comments. But I pushed the re-review button anyway in just in case. However, I'm targeting a release by EOD, so if you don't have time to check it out by then, I will assume this is fine and merge as part of that release process. |
| console.print(f"[bold red]Error[/bold red] executing Python script {script_path}:") | ||
| console.print(e.stderr.decode(), style="red") | ||
| print(f"Error executing Python script {script_path}:", file=stderr) | ||
| sys.stderr.write(e.stderr.decode()) |
There was a problem hiding this comment.
Just for my curiosity - why not use print here?
There was a problem hiding this comment.
My very brief thinking here was that since the string being printed was not UI copy defined in the code, but rather was a value being provided by other bits of API, then sys.stderr.write communicated more clearly to the reader that this code is simply conveying the value directly without any modifications, such as the newline automatically added by print.
Of course I could suppress that and do print(e.stderr.decode(),end='',file=stderr). This has the pro of being more consistent with the other uses of print, but the con of being a little more roundabout.
So this seemed a little cleaner? But if I'd put more sugar in my coffee in the morning, I probably would have gone the other way. Borderline case!
There was a problem hiding this comment.
Well, you then immediately print a newline on the next line, so you end up with two lines instead of one IIUC…
FWIW, for this reader at least, sys.stderr.write communicated to me that you're doing something special/different, and I should try to figure out why! :D
|
lgtm @algal :) |
sys.stderr.write.