Details
- Reviewers
joshuaspence epriestley - Group Reviewers
Blessed Reviewers - Maniphest Tasks
- T9029: $default_no is misleading / potentially wrong for PhutilConsole::confirm in various workflows
- Commits
- rARC807057087d65: Restore default "yes" behavior for lint patch
Run arc lint on a repository where patch is suggested. Default
option should be "[Y/n]" to accept the patch.
Diff Detail
- Repository
- rARC Arcanist
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
I think this is reasonable, and that fixing callsites is desirable rather than changing the method (that is, a signature which accepts $default is clearer than a signature which accepts $default_no -- hopefully that was my reasoning in changing the signature, although I probably just made a mistake). If you want to run down the rest of these, I'm happy to take a look at them. (I don't imagine there are all that many.)
It looks like this is the only one with $console->confirm() and $default_no.
There are other instances of $default_no = false but those are still using phutil_console_confirm. Should we (I) update those to $console->confirm() or is that just a yak to shave?
I'd say it probably isn't worth pursuing for now.
This $console stuff sort of ended up in limbo. It was originally added to let us run lint and unit tests in the background while prompting the user for a commit message, but that whole endeavor was of questionable value and created a lot of unreproducible, impossible-to-resolve bugs where things would hang indefinitely. I eventually got rid of it, which resolved all the weird hangs, but it made the $console abstraction sort of pointless.
It isn't clear that there's anything else we can really do with it, but it was a lot of work to add it and it seems like it's probably good to have it, but it's plausible that we might just rip it out at some point so we can junk all the associated code. Since it's not totally clear which way is "forward", probably best to let sleeping dogs lie.