Vamsi-klu commented on PR #63579: URL: https://github.com/apache/airflow/pull/63579#issuecomment-4061827715
Hey @bugraoz93, appreciate you taking the time to share your thoughts — that's a fair concern in general about `while True`. I've run into cases myself where a missed exit condition in a loop caused headaches, especially in background services or networking code where you can end up with a silent busy-wait. That said, I think the context here makes it a pretty safe choice: - `_read_char()` is a blocking call that waits on TTY input (or `inputimeout` in CI), so each iteration either returns immediately on valid input or blocks until the user presses something. There's no spinning or CPU burn happening. - `prompt_triage_action` right below in the same file uses `while True` for the exact same re-prompt-on-invalid-key pattern. Having two different patterns for the same behavior in the same file would be a bit confusing for anyone reading the code later. - I did think about what a flag-based version would look like — something like `valid = False; while not valid:` — but since every recognized key path does a `return`, the flag would never actually flip to `True`. It'd just sit there unused. You could restructure to store the result and `break`, but that adds a variable and a branch for no real clarity gain here. And yeah, fair enough on the AI comment — I do use AI tools as part of my workflow (I think most of us do these days to some degree). But I did read through the file and trace the code paths before commenting. Things like the merge conflict with `console_print()` and pointing to `prompt_triage_action` as precedent came from actually looking at the code, not from a prompt. Happy to dig into any specific point further if you think there's something I missed! -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
