Page MenuHomePhabricator

Display informative message when arc launches an editor
ClosedPublic

Authored by cspeckmim on Jul 20 2021, 9:58 PM.

Details

Summary

Update PhutilInteractiveEditor to allow specifying a "task message" which will be displayed just prior to launching the user's editor.

Refs T3271

Test Plan

I ran several arc diff commands in varying states to invoke my editor and verified that it printed out the text indicating that my editor was being launched.

Diff Detail

Repository
rARC Arcanist
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

src/console/PhutilInteractiveEditor.php
85–87

I'm not super enthusiastic about this text formatting but I think it's the simplest way of setting this up without having the different call sites also have to retrieve the editor name to include it in the message, while also handling localization in a reasonably translatable format without concatenating strings.

I think the alternative is exposing the editor name from here and having each thing that invokes ArcanistWorkflow::newInteractiveEditor() to construct the phrasing and pass it to that function.

Maybe the output could be two separate lines:

Launching editor ("nano")...
Supply commit message for uncommitted changes, then save and exit.

Then neither part needs to care about the other part, and things are likely more translatable and such.

In this mode, it might also be helpful to print a success message ("Edit completed.")? I'm on the fence about this, but it might make debugging user reports easier since you can tell if they made it through the editing part or not -- not sure how much confusion/ambiguity you've run into in practice.

Update the display format to not try and combine two things

Maybe the output could be two separate lines:

Launching editor ("nano")...
Supply commit message for uncommitted changes, then save and exit.

Then neither part needs to care about the other part, and things are likely more translatable and such.

Yea that works a lot better. Though I'm on the fence about the order of the two messages. Logically my brain wants the "Launching..." message to be second but I don't think it really matters

In this mode, it might also be helpful to print a success message ("Edit completed.")? I'm on the fence about this, but it might make debugging user reports easier since you can tell if they made it through the editing part or not -- not sure how much confusion/ambiguity you've run into in practice.

Yea I could add that too. Is there any precedent for nested/indented messages? Something like

Launching editor ("nano")...
  Supply commit message for uncommitted changes, then save and exit.
  Edit completed.

I don't think I've run into this issue at all, but saw T3271 referenced from PHI1667 and figured this would be a good simple task to try out.

There a bit of fancier indent/formatting stuff in some of the newer code (e.g., in ArcanistRefView->newLines()) but it hasn't really generalized into something you could easily apply here yet. I think this is fine for now and we could revisit it to make it fancier later on if the newest display stuff gets generalized a bit.

src/console/PhutilInteractiveEditor.php
83

For consistency, prefer:

echo tsprintf(
  "%s\n",
  pht('Launching editor "%s"...', $binary));

That is:

  • Keep "\n" out of the translatable string since it's more of a formatting thing.
  • Use double quotes to identify values in human-readable text. That is, prefer:

Press "F" to pay respects.

...over:

Press 'F' to pay respects.

Existing code, particularly older code, sometimes uses single quotes (and very old code may use backticks, or even monospaced-curly-quotes (` ... ')), but new code has used double quotes consistently for some time. I think this generally scans better as a human reader, and the major advantage of single quotes is that they require less escaping in code, which probably shouldn't be the controlling argument at the display layer.

This revision is now accepted and ready to land.Jul 20 2021, 10:58 PM
src/console/PhutilInteractiveEditor.php
83

Keep "\n" out of the translatable string since it's more of a formatting thing.

Blerp that makes sense

Use double quotes to identify values in human-readable text. That is...

Ah okay I'll keep that in mind to be consistent. I've certainly been inconsistent previously, probably just choosing whichever quote doesn't need escaped in the string I need without paying too much attention to it.

This revision was automatically updated to reflect the committed changes.
cspeckmim marked an inline comment as done.