Using Robo Runner #1179

TeslaDethray · 2016-09-08T19:31:57Z

Extracted from #1177

dustinleblanc · 2016-09-08T19:40:18Z

src/Terminus.php

     */
-    protected function getDefaultInputDefinition()
+    private function setRunner()


A setter should probably explicitly accept an argument, it will be easier to test and also refactor later

dustinleblanc · 2016-09-08T21:24:39Z

injecting those dependencies like a boss!

greg-1-anderson · 2016-09-08T21:41:29Z

src/Commands/TerminusCommand.php

    {
-        $this->config = $config;
+        $this->config = new Config();
+        $this->setLog(new ConsoleLogger($this->output()));


Might want to inject a logger; I'd think you would definitely want to inject the Config() object, since setting that up probably involves parsing files.

How can I inject things into the commands Robo\Runner instantiates?

That's a reasonable request! ;)

Robo is designed to use league/container inflection to automatically inject your dependencies. Just make your class instantiate a SomethingAwareInterface, and the inflector will call the appropriate set method provided by SomethingAwareTrait in the same class.

However, you should not be REQUIRED to use league/container to use the Robo runner. I'll make a small change to the runner to allow you to pass in already-constructed command classes.

Also, this line ($this->setLog(new ConsoleLogger($this->output()));) is the source of the verbosity bug. What's happening here is that Robo injects the output object via the technique above ^^; it uses setOutput() in OutputAwareTrait, since you implement OutputAwareInterface. The result of this is that $this->output() is not available in your constructor, as the constructor is called to set up the object before its setOutput() method is called.

n.b. if you construct your own objects, you'll need to inject the input and output objects yourself.

I should also mention that Robo sets up its own logger object, so if you just make TerminusCommand implement LoggerAwareInterface, then Robo will inject a working logger for you, available via protected $logger;, which is in LoggerAwareTrait.

use Psr\Log\LoggerAwareInterface;
use Psr\Log\LoggerAwareTrait;

Aha, THIS is just the thing I needed! WOOT!

TeslaDethray · 2016-09-09T00:27:52Z

src/Commands/TerminusCommand.php

    {
-        $this->config = $config;
+        $this->config = new Config();


Injecting this means that I won't be able to let Robo work its inputter/outputter/logger-injecting magic.

Would using an ioc container make a difference? My brain starts to hurt when I think about service containers (and the yaml loading mess of Symfony's container makes it worse). If an upstream change in Robo would make it easier, we could submit a PR there too. For now, newing up an object in the constructor isn't the end of the world, but we should fight it like the plague before we take on too much tech debt.

It's really not that hard. Robo's injecting magic is designed to be extensible.

Make yourself a new TerminusConfigAwareInterface / Trait.

Make TerminusCommand implement the interface and use the trait.

Share your Config singleton in the container.

Add an inflector to the league/container at startup to set your config object on anything that implements TerminusConfigAwareInterface.

For that matter, you might be able to just get away with using Robo's config object. It has getters and setters; might work. Or, subclass it and use Robo's ConfigAwareInterface / ConfigAwareTrait.

Yes, Robo rocks. I'll show you how to do (3) and (4) tomorrow. It's just a few easy lines of code, because league/container rocks. It's a pity that so many people try to learn DI with the Symfony container, because I agree that Symfony service containers are prone to making heads hurt. But it doesn't have to be painful.

dustinleblanc · 2016-09-09T01:01:51Z

src/Runner.php

-            $subdirectories = array_merge($subdirectories, $this->locateSubdirectories($dir, $next_namespace));
-        }
-        return $subdirectories;
+        $commands = array_filter(


I <3 this functional waterfall of delight

I love it too -- even though I feel bad that you had to write it. ;) You can take it out when you update to annotated commands 1.3.1 and set the search locations to empty instead of ['.'].

coveralls · 2016-09-09T01:28:23Z

Coverage increased (+1.9%) to 15.928% when pulling 691a281 on change/robo_runner into b588e6b on master.

greg-1-anderson · 2016-09-09T04:14:46Z

src/Runner.php

-                }
-            }
-        }
+        $discovery->setSearchPattern('*Command.php')->setSearchLocations(['.']);


I'll give you a new CommandFileDiscovery that will work right if you change this to setSearchLocations([]);. The problem with . as a search location is it makes the command discovery object search each location twice, because it always searches the base directory atm.

I pushed a new consolidation/annotated-command version 1.3.1 that supports this.

… objects. Demonstrate verbose/debug output with some temporary code in the Art command.

greg-1-anderson · 2016-09-09T16:59:10Z

src/Config.php

@@ -228,6 +218,22 @@ private static function getTerminusScript()
    }

    /**
+     * Ensures that directory paths work in any system


I think this might blow up Cygwin, but I haven't used Cygwin in many years and don't quite remember.

Hopefully with bash support in Win10 on it's way we will have less cause to care 😀

I have Cygwin installed on a VirtualBox, so let me know if you'd like me to test.

greg-1-anderson · 2016-09-09T17:14:43Z

I pushed a commit to improve DI here. I will also fix up the unit tests, but first I want to make the Robo API a bit better, so we can clean up things here a little more.

coveralls · 2016-09-09T19:37:49Z

Coverage remained the same at 14.004% when pulling 223eda4 on change/robo_runner into b588e6b on master.

TeslaDethray · 2016-09-09T19:40:18Z

I fixed a couple bugs, brushed up the internal docs, and fixed the unit tests. Although there are still a few things that can be done to further enhance Terminus 1.0's use of Robo, I think this PR should be merged before we have the time to discuss those changes because it is blocking the work of others.

ronan · 2016-09-09T19:44:39Z

This is looking sharp. I think any improvements will be minor and don't need to block this PR.

👍 👍 👍

bensheldon · 2016-09-09T19:47:14Z

👍 excited for the 🛫

TeslaDethray added Changes Requested Needs Review labels Sep 8, 2016

TeslaDethray self-assigned this Sep 8, 2016

TeslaDethray force-pushed the change/robo_runner branch 2 times, most recently from 1cad5ed to 7a43ec6 Compare September 8, 2016 19:32

dustinleblanc reviewed Sep 8, 2016
View reviewed changes

TeslaDethray force-pushed the change/robo_runner branch 6 times, most recently from d58f94f to a01086c Compare September 8, 2016 20:52

TeslaDethray force-pushed the change/robo_runner branch from a01086c to 309d881 Compare September 8, 2016 21:28

greg-1-anderson reviewed Sep 8, 2016
View reviewed changes

TeslaDethray force-pushed the change/robo_runner branch 5 times, most recently from d8a216a to fdde1e5 Compare September 9, 2016 00:24

TeslaDethray reviewed Sep 9, 2016
View reviewed changes

TeslaDethray force-pushed the change/robo_runner branch 3 times, most recently from ff888f1 to 3a8f258 Compare September 9, 2016 00:32

dustinleblanc reviewed Sep 9, 2016
View reviewed changes

TeslaDethray force-pushed the change/robo_runner branch from 3a8f258 to 411ec1b Compare September 9, 2016 01:09

Changed to using Robo's runner

691a281

TeslaDethray removed the Changes Requested label Sep 9, 2016

TeslaDethray force-pushed the change/robo_runner branch from 411ec1b to 691a281 Compare September 9, 2016 01:21

greg-1-anderson reviewed Sep 9, 2016
View reviewed changes

Use Robo dependency injection to set up Symfony Style and Robo Logger…

6a4d1d6

… objects. Demonstrate verbose/debug output with some temporary code in the Art command.

greg-1-anderson reviewed Sep 9, 2016
View reviewed changes

ronan mentioned this pull request Sep 9, 2016

[WIP] Adds machine token commands for 1.0 #1182

Merged

Fixed missing-var bug, brushed up internal documentation

223eda4

TeslaDethray merged commit fa9281a into master Sep 9, 2016

TeslaDethray deleted the change/robo_runner branch September 9, 2016 19:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Using Robo Runner #1179

Using Robo Runner #1179

TeslaDethray commented Sep 8, 2016

dustinleblanc Sep 8, 2016

dustinleblanc commented Sep 8, 2016

greg-1-anderson Sep 8, 2016

TeslaDethray Sep 8, 2016

greg-1-anderson Sep 8, 2016

greg-1-anderson Sep 8, 2016

greg-1-anderson Sep 8, 2016

greg-1-anderson Sep 8, 2016

TeslaDethray Sep 9, 2016

TeslaDethray Sep 9, 2016

dustinleblanc Sep 9, 2016

greg-1-anderson Sep 9, 2016

dustinleblanc Sep 9, 2016

greg-1-anderson Sep 9, 2016

coveralls commented Sep 9, 2016

greg-1-anderson Sep 9, 2016

greg-1-anderson Sep 9, 2016

greg-1-anderson Sep 9, 2016

dustinleblanc Sep 9, 2016

ari-gold Sep 9, 2016

greg-1-anderson commented Sep 9, 2016

coveralls commented Sep 9, 2016 •

edited

TeslaDethray commented Sep 9, 2016

ronan commented Sep 9, 2016

bensheldon commented Sep 9, 2016

Using Robo Runner #1179

Using Robo Runner #1179

Conversation

TeslaDethray commented Sep 8, 2016

Choose a reason for hiding this comment

dustinleblanc commented Sep 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Sep 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg-1-anderson commented Sep 9, 2016

coveralls commented Sep 9, 2016 • edited

TeslaDethray commented Sep 9, 2016

ronan commented Sep 9, 2016

bensheldon commented Sep 9, 2016

coveralls commented Sep 9, 2016 •

edited