Changeset View
Standalone View
src/toolset/ArcanistWorkflow.php
| <?php | <?php | ||||
| abstract class ArcanistWorkflow extends Phobject { | abstract class ArcanistWorkflow extends Phobject { | ||||
| private $runtime; | |||||
| private $toolset; | private $toolset; | ||||
| private $arguments; | private $arguments; | ||||
| private $configurationEngine; | private $configurationEngine; | ||||
| private $configurationSourceList; | private $configurationSourceList; | ||||
| /** | /** | ||||
| * Return the command used to invoke this workflow from the command like, | * Return the command used to invoke this workflow from the command like, | ||||
| * e.g. "help" for @{class:ArcanistHelpWorkflow}. | * e.g. "help" for @{class:ArcanistHelpWorkflow}. | ||||
| Show All 19 Lines | abstract class ArcanistWorkflow extends Phobject { | ||||
| * @param ArcanistToolset Current selected toolset. | * @param ArcanistToolset Current selected toolset. | ||||
| * @return bool True if this command supports the provided toolset. | * @return bool True if this command supports the provided toolset. | ||||
| */ | */ | ||||
| public function supportsToolset(ArcanistToolset $toolset) { | public function supportsToolset(ArcanistToolset $toolset) { | ||||
| // TOOLSETS: Temporary! | // TOOLSETS: Temporary! | ||||
| return true; | return true; | ||||
| } | } | ||||
| protected function getWorkflowArguments() { | |||||
| // TOOLSETS: Temporary! | |||||
| return array(); | |||||
| } | |||||
| protected function getWorkflowInformation() { | |||||
| // TOOLSETS: Temporary! | |||||
| return null; | |||||
| } | |||||
| public function newPhutilWorkflow() { | public function newPhutilWorkflow() { | ||||
| return id(new ArcanistPhutilWorkflow()) | $arguments = $this->getWorkflowArguments(); | ||||
| assert_instances_of($arguments, 'ArcanistWorkflowArgument'); | |||||
| $specs = mpull($arguments, 'getPhutilSpecification'); | |||||
| $phutil_workflow = id(new ArcanistPhutilWorkflow()) | |||||
| ->setName($this->getWorkflowName()) | ->setName($this->getWorkflowName()) | ||||
| ->setWorkflow($this); | ->setWorkflow($this) | ||||
| ->setArguments($specs); | |||||
| $information = $this->getWorkflowInformation(); | |||||
| if ($information) { | |||||
| $examples = $information->getExamples(); | |||||
| if ($examples) { | |||||
| $examples = implode("\n", $examples); | |||||
| $phutil_workflow->setExamples($examples); | |||||
| } | |||||
| $help = $information->getHelp(); | |||||
| if (strlen($help)) { | |||||
| // Unwrap linebreaks in the help text so we don't get weird formatting. | |||||
| $help = preg_replace("/(?<=\S)\n(?=\S)/", " ", $help); | |||||
amckinley: Is this so fancy because you're trying to avoid removing trailing or leading newlines? Why not… | |||||
Done Inline ActionsThe "help" is a big block of text like this: Blah blah blah blah blah blah blah blah blah blah blah. **Helpful Heading** Blah blah this command: $ blah blah blah Blah blah blah blah I want to remove only one newline from this example, "unwrapping" this line while retaining all the formatting newlines: Blah blah blah blah blah blah <<< Remove This Newline Only
blah blah blah blah blah.
**Helpful Heading**
Blah blah this command:
$ blah blah blah
Blah blah blah blahWith the implode/explode or preg_replace approaches, we'd get rid of all the newlines. The "clever regexp" approach only removes newlines surrounded on either side by non-whitespace, so only "unwrappable" lines. This isn't great. A better solution might be to put the help in separate text files or something, but some of it currently takes a lot of "%s" parameters. That also makes it more difficult to translate. I'm not really super happy about this piece of code but I'm not sure how we can do better without adding a lot of weirdness. We could also disable the separate wrapping inside the Phutil part of help-printing, and maybe that's cleaner. However, it means we can't indent/wrap this block into a format other than the written-in-PHP format. I'm not sure if I want to or not. I'm inclined to just note this and revisit it later? epriestley: The "help" is a big block of text like this:
```
Blah blah blah blah blah blah
blah blah blah… | |||||
Not Done Inline ActionsI would have assumed that <<EOF-style strings were the way to go, but I can also see this^^ being less work than converting a bunch of existing multi-line string concatenation. amckinley: I would have assumed that `<<EOF`-style strings were the way to go, but I can also see this^^… | |||||
Done Inline ActionsThe workflows are using <<<EOF strings, but newlines are respected in them -- and they're currently written with newlines since they're in code and lint will complain later if they aren't wrapped to 80 columns. epriestley: The workflows are using `<<<EOF` strings, but newlines are respected in them -- and they're… | |||||
Not Done Inline ActionsAlso, I now want tooling that automatically generates inline regexr links for arguments passed to preg_replace. amckinley: Also, I now want tooling that automatically generates inline [[ https://regexr.com/3vupn |… | |||||
| $phutil_workflow->setHelp($help); | |||||
Not Done Inline ActionsI guess this is just a style thing, but I definitely would have put this^^ logic inside setHelp(). I've seen you use this pattern in a lot of other places; any particular reason? amckinley: I guess this is just a style thing, but I definitely would have put this^^ logic inside… | |||||
Done Inline ActionsI generally think this should be true: $x = <some value>; $object->setThing($x); $y = $object->getThing(); assert($y === $x); That is, if you setThing() and then getThing(), you should (usually, probably, almost) always get exactly the same thing back. This isn't good for any particular "legal" reason, but if getThing or setThing include a hidden mutation, I think it tends to lead to code that violates expectations in some subtle way sooner or later. Even in the best case where no one makes a mistake, sometimes you discover you actually need the raw value. Then you're in trouble if you can't rename getThing() safely, since you have to add a getThingRawValue(). A different soft rule I try to follow that I can't really put my finger on a concise explanation of is something like "kind of preserve raw inputs for a long time unless there's a pretty good reason not to or this rule conflicts with a better rule?". Not much of a rule. But when something goes wrong, it's often easier to debug if the error can say "we started with input X and a bad thing happened" than if it it has to say "we started with some input, and then mutated it, and then ended up with Y, and then a bad thing happened". If you know "input X" made a bad thing happen, you can usually go put "$input = X" in the code somewhere and reproduce instantly. If you only know that mystery input X which can be mutated into Y makes bad things happen, you sometimes have to walk up the stack a step at a time before the thing reproduces. I did consider putting this logic inside WorkflowInformation as a method like getUnwrappedHelpForDisplay() instead of here in Workflow, and that's the approach I usually prefer (that is, getX() and getXHeyBuddyThisMethodMutatesThings() feel preferable to getX() and getRawX() to me) but that class is pretty clean and this class is a big mess so I just threw it on the pile here instead. Broadly, yes, this block is sketchy and at least merits a revisit later. epriestley: I //generally// think this should be true:
```
$x = <some value>;
$object->setThing($x);
$y =… | |||||
Not Done Inline ActionsThis was really interesting; thanks for the write up! amckinley: This was really interesting; thanks for the write up! | |||||
| } | |||||
| } | |||||
| return $phutil_workflow; | |||||
| } | } | ||||
| final public function getToolset() { | final public function getToolset() { | ||||
| return $this->toolset; | return $this->toolset; | ||||
| } | } | ||||
| final public function setToolset(ArcanistToolset $toolset) { | final public function setToolset(ArcanistToolset $toolset) { | ||||
| $this->toolset = $toolset; | $this->toolset = $toolset; | ||||
| return $this; | return $this; | ||||
| } | } | ||||
| final public function setRuntime(ArcanistRuntime $runtime) { | |||||
| $this->runtime = $runtime; | |||||
| return $this; | |||||
| } | |||||
| final public function getRuntime() { | |||||
| return $this->runtime; | |||||
| } | |||||
| final public function getConfig($key) { | |||||
| return $this->getConfigurationSourceList()->getConfig($key); | |||||
| } | |||||
| final public function setConfigurationSourceList( | final public function setConfigurationSourceList( | ||||
| ArcanistConfigurationSourceList $config) { | ArcanistConfigurationSourceList $config) { | ||||
| $this->configurationSourceList = $config; | $this->configurationSourceList = $config; | ||||
| return $this; | return $this; | ||||
| } | } | ||||
| final public function getConfigurationSourceList() { | final public function getConfigurationSourceList() { | ||||
| return $this->configurationSourceList; | return $this->configurationSourceList; | ||||
| Show All 31 Lines | final public function executeWorkflow(PhutilArgumentParser $args) { | ||||
| if ($caught) { | if ($caught) { | ||||
| throw $caught; | throw $caught; | ||||
| } | } | ||||
| return $err; | return $err; | ||||
| } | } | ||||
| final public function getArgument($key, $default = null) { | final public function getArgument($key) { | ||||
| // TOOLSETS: This is a stub for now. | return $this->arguments->getArg($key); | ||||
| return $default; | } | ||||
| final protected function newWorkflowArgument($key) { | |||||
| return id(new ArcanistWorkflowArgument()) | |||||
| ->setKey($key); | |||||
| } | |||||
| return $this->arguments->getArg($key, $default); | final protected function newWorkflowInformation() { | ||||
| return new ArcanistWorkflowInformation(); | |||||
| } | } | ||||
| } | } | ||||
Is this so fancy because you're trying to avoid removing trailing or leading newlines? Why not implode(" ", explode("\n", $help)); or preg_replace("/\n/", " ", $help);?