Page MenuHomePhabricator

Restore default "yes" behavior for lint patch
ClosedPublic

Authored by sectioneight on Aug 11 2015, 9:40 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Mar 26, 3:45 PM
Unknown Object (File)
Sun, Mar 10, 7:38 PM
Unknown Object (File)
Feb 14 2024, 1:45 AM
Unknown Object (File)
Feb 3 2024, 6:16 AM
Unknown Object (File)
Feb 3 2024, 6:16 AM
Unknown Object (File)
Feb 3 2024, 6:16 AM
Unknown Object (File)
Jan 28 2024, 8:09 AM
Unknown Object (File)
Jan 28 2024, 8:09 AM
Subscribers

Details

Summary

Fixes T9029

See T9029 for more details, but basically at some point phutil_console_confirm
(which takes a $default_no parameter) was refactored to $console->confirm()
which takes a $default parameter with semantics negated..

Test Plan

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

sectioneight retitled this revision from to Restore default "yes" behavior for lint patch.
sectioneight updated this object.
sectioneight edited the test plan for this revision. (Show Details)
sectioneight added a reviewer: joshuaspence.
NOTE: If this is the correct strategy and I'm not confused, there are other $default_no = false instances that should be fixed.
epriestley added a reviewer: epriestley.

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.)

This revision is now accepted and ready to land.Aug 11 2015, 9:49 PM
This revision was automatically updated to reflect the committed changes.

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.

Sounds good.

/me disappears, for now