-
Notifications
You must be signed in to change notification settings - Fork 25
Conversation
356a5bc
to
3ba01b6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, this looks like a nice addition. I'll take care of the changes for which I left feedback during merge.
Thanks, @bmiklautz !
src/Auto.php
Outdated
/** | ||
* @var value of COMPOSER_DEV_MODE | ||
*/ | ||
private $composer_dev_mode; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use camelCase for property names.
src/Auto.php
Outdated
} | ||
} | ||
// not running under composer | ||
echo 'COMPOSER_DEV_MODE not set. Nothing to do.' . PHP_EOL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please invert the statements, so that we can return early. As an example:
if ($this->composerDevMode === '') {
echo 'COMPOSER_DEV_MODE not set; nothing to do.' . PHP_EOL;
return 0;
}
if ($this->composerDevMode === '0') {
$disable = $this->projectDir, $this->errorStream);
return $disable();
}
$enable = new Enable($this->projectDir, $this->errorStream);
return $enable;
Changes like this make it easier to determine the various behaviors, boiling this one down to exactly three.
src/Command.php
Outdated
@@ -42,6 +42,9 @@ public function __invoke(array $arguments) | |||
case 'status': | |||
$status = new Status(); | |||
return $status(); | |||
case 'auto_composer': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We typically use dash-separated names, so this should become auto-composer
.
src/Auto.php
Outdated
|
||
namespace ZF\DevelopmentMode; | ||
|
||
class Auto |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should rename this class to mimic the command itself: AutoComposer
.
When auto_composer is used development mode is set based on the environment Variable COMPOSER_DEV_MODE. If it's undefined the mode isn't touched. If its anything else then 0 developer mode is enabled. This can be used as post-[install|update]-cmd for composer to enable development mode based on the mode composer was called with (--dev vs. --no-dev): ... "scripts": { "development-auto": "zf-development-mode auto_composer", "post-install-cmd": ["@development-auto" ], "post-update-cmd": [ "@development-auto" ], } ...
* move functionality to an extra class * add the corresponding tests
- Renames class to `AutoComposer` - Renames command to `auto-composer` - Rewrites `AutoComposer::__invoke()` to do a conditional per behavior, with each returning. In doing so, identified an issue with how it detected absence of a `COMPOSER_DEV_MODE` value, as well as inability to correctly report an invalid value was provided.
3ba01b6
to
4032492
Compare
When auto_composer is used development mode is set based on the
environment Variable COMPOSER_DEV_MODE. If it's undefined the mode
isn't touched. If its anything else then 0 developer mode is enabled.
This can be used as post-[install|update]-cmd for composer to enable
development mode based on the mode composer was called with (--dev vs. --no-dev):
...
"scripts": {
"development-auto": "zf-development-mode auto_composer",
"post-install-cmd": ["@development-auto" ],
"post-update-cmd": [ "@development-auto" ],
}
...