Port simplesamlphp targets to Robo. #1485
Conversation
e964a0a to
c5f38ef
Compare
8f23fd5 to
47ee379
Compare
3b045f2 to
51b2ee0
Compare
| $composerBin = $this->getConfigValue('composer.bin'); | ||
| $projectConfigFile = $this->getConfigValue('blt.config-files.project'); | ||
| $this->say("Updating ${projectConfigFile}..."); | ||
| $result = $this->taskExec("{$composerBin}/yaml-cli update:value") |
There was a problem hiding this comment.
We should probably just use Yaml CLI's classes to do this more directly.
There was a problem hiding this comment.
Hmm would that look like? The classes I see provided by grasmash/yaml-cli are all command files with protected methods (unless I'm looking in the wrong place).
| * @return bool | ||
| */ | ||
| public function isSimpleSamlPhpInstalled() { | ||
| return $this->getConfig()->has('simplesamlphp') && $this->getConfigValue('simplesamlphp'); |
There was a problem hiding this comment.
Please move this to the Inspector class. This command should be able to access the method via $this->getInspector()->isSimpleSamlPhpInstalled();
| $this->say('SimpleSAMLphp has already been initialized by BLT.'); | ||
| } | ||
| $this->simpleSamlPhpInitComplete(); | ||
| if (isset($result)) { |
There was a problem hiding this comment.
We should returns something in all instances.
| * @return \Robo\Result | ||
| * @throws \Exception | ||
| */ | ||
| protected function simpleSamlPhpLibInit() { |
There was a problem hiding this comment.
The method names don't all need to start with simpleSamlPhp, since this file is dedicated to that namespace. I'd suggest something more descriptive like requirePackages() or requireModule()
There was a problem hiding this comment.
oh right - fixing that
| * | ||
| * @command simplesamlphp:config:init | ||
| */ | ||
| public function simpleSamlPhpConfigInit() { |
There was a problem hiding this comment.
Please also begin method names with verbs where possible. E.g., initializeConfig().
| * @command simplesamlphp:build:config | ||
| */ | ||
| public function simpleSamlPhpBuildConfig() { | ||
| $this->say('Copying config files to the appropriate place in simplesamlphp library.'); |
There was a problem hiding this comment.
For statements using the present participle, please end with an ellipsis.
| * | ||
| * @return \Robo\Result | ||
| * | ||
| * @command simplesamlphp:build:config |
There was a problem hiding this comment.
Please make @command the first annotation.
| $this->say(" | ||
|
|
||
|
|
||
| ============================================================================ |
There was a problem hiding this comment.
I'm not crazy about the usage of the === as a separator here or the hardcoding of whitespace, since we don't do anything else like this in BLT. I'd prefer to use Symfony Console formatting if necessary. E.g., use <comment> or <info> wrappers to colorize, and blocks if necessary.
There was a problem hiding this comment.
Yeah - I honestly just copy/pasted from the phing project - I'll fix this up
|
@malikkotob Well done! I've added some comments, largely requesting stylistic changes. |
51b2ee0 to
17a27e9
Compare
|
@grasmash I've addressed everything but the Yaml CLI issue - let me know your thoughts on my question on that |
17a27e9 to
b2d657c
Compare
b2d657c to
15f63d3
Compare
No description provided.