Page MenuHomePhabricator

arc set-config: warn about unknown keys
ClosedPublic

Authored by avivey on Feb 15 2017, 1:21 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Dec 1, 9:54 PM
Unknown Object (File)
Fri, Nov 29, 6:19 PM
Unknown Object (File)
Fri, Nov 29, 9:58 AM
Unknown Object (File)
Tue, Nov 26, 9:16 PM
Unknown Object (File)
Tue, Nov 26, 7:39 AM
Unknown Object (File)
Tue, Nov 26, 4:05 AM
Unknown Object (File)
Fri, Nov 22, 7:09 PM
Unknown Object (File)
Fri, Nov 22, 8:06 AM

Details

Summary

See T12266. Warn when the user tries to set a value we will probably not read.

Test Plan

arc set-config foo bar, see fancy warning.

Diff Detail

Repository
rARC Arcanist
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

epriestley added inline comments.
src/workflow/ArcanistSetConfigWorkflow.php
79

I think you can make this translatable like this:

$warning = tsprintf(
  "**%s:** %s\n",
  pht('Warning'),
  pht('The configuration key ...'));
$console->writeErr("%s", $warning);
This revision is now accepted and ready to land.Feb 15 2017, 1:29 PM
This revision was automatically updated to reflect the committed changes.
joshuaspence added inline comments.
src/workflow/ArcanistSetConfigWorkflow.php
103

Hmm, now these strings aren't translatable.

I just assumed that writeOut() and friends are a flavor of pht()...

pht() is the only thing that does translations right now.

Omitting tsprintf() means that ANSI stuff won't be escaped. It doesn't really matter here since the string is controlled by the user, but there are quasi-attacks where users create a task or revision with a lot of terminal control characters in it so that arc list or arc todo or whatever potentially take over your terminal. tsprintf() avoids that.

The overall state of affairs isn't great right now since the minimal amount of code you need to write the word "hello" to stderr "correctly" (so it can be translated and is ANSI-safe) is this big mess:

$console->writeErr(
  "%s",
  tsprintf(
    "%s\n",
    pht('hello')));

This is pretty silly, but since pht() needs to be statically extracted we can't easily get rid of it, and merging the $console stuff with tsprintf() is sort of half-pointless if we don't go all the way to try to get rid of the pht() too.

There's also at least some kind of argument that "\n" and ANSI sequences shouldn't be translatable: at best, they're probably pretty confusing.