bugraoz93 commented on PR #63579: URL: https://github.com/apache/airflow/pull/63579#issuecomment-4061421619
> Hey @choo121600, good find — accidentally approving workflow runs by pressing a random key is definitely a safety concern for the triage tool. The `while True` loop approach is the right fix and it matches how `prompt_triage_action` in the same file already handles this. > > One thing to watch out for: there's a **merge conflict** with current main. Commit 7cea0b83ac landed a refactor that replaced `get_console().print()` with `console_print()` across the codebase. You'll need to rebase and update those calls. > > A couple of other thoughts: > > - **Tests would help here.** There are no existing tests for `user_confirm`, and since the behavioral change is significant (re-prompt instead of silent default), even a couple of targeted tests would go a long way. For example, mocking `_read_char` to return "x" then "y" and asserting `Answer.YES` would prove unrecognized keys trigger a re-prompt. Same for multi-byte escape sequences. > > - **Reviewer @bugraoz93 suggested a flag approach** instead of `while True` — I think `while True` is the right call here since it's idiomatic Python for this pattern and matches the existing code in the same file. Might be worth responding to that comment to close the loop. > > The logic itself looks correct to me across all the edge cases I checked (Enter with/without default, case sensitivity, quit_allowed=False, special characters). Clean and well-scoped fix. I don't know why AI goes with `while true` which generally hard to maintain because it is always error prune. You always need to break correctly and handle exceptions properly. Otherwise process can do busy waits that can cause harder debugging. You wait process to throw exceptions but waiting 15 minutes on `while true` because one if statement wasn't properly set. Maybe Python makes some parts more foregiving because it wraps one additional layer on top of `c-assembly-hex` interpretion. This doesn't directly make `while true` is a good approach in general. As your entire message looks fully AI generated, so I assume AI suggested the approach without you spending any time on reviewing the actual code to easily say it is the best approach to use `while true` while I am spending more time explaining AI my reasoning :D -- 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]
