New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Override couscous.yml configuration from command line #187
Conversation
Added to |
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.
That looks like a very good start! I've commented on minor things, if you can add tests that would be great!
'config', | ||
null, | ||
InputOption::VALUE_REQUIRED | InputOption::VALUE_IS_ARRAY, | ||
'If specified will override entries in couscous.yaml (key=value)', |
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.
Typo: couscous.yaml
-> couscous.yml
|
||
$project = new Project($sourceDirectory, getcwd().'/.couscous/generated'); | ||
|
||
$project->metadata['tempConfigRaw'] = $tempConfigRaw; |
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.
tempConfigRaw
doesn't sound ideal to me, how about something like cliConfig
?
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.
cliConfig
works, albeit a bit clunky to read. I think tempConfigRaw
was a leftover name from when I was storing the processed array in tempConfig
and wanted to differentiate.
$this->logger->notice('Overriding global config: '.$explosion[0].' = "'.$explosion[1].'"'); | ||
|
||
$keys[] = $explosion[0]; | ||
$values[] = $explosion[1]; |
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.
Instead of using 2 arrays wouldn't it be possible to use one and do:
$config[$explosion[0]] = $explosion[1];
?
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.
Yes, it would! I'm just a silly. :)
|
||
public function __invoke(Project $project) | ||
{ | ||
if ($project->metadata['tempConfigRaw']) { |
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.
Minor detail but you can return early to avoid 1 level of indentation
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.
Good tip!
Fix typo, rename tempConfigRaw to cliConfig, some performance tweaks.
Fix misnamed variable.
Contains 3 test cases (well-formed array, empty array, missing option)
Added three tests for |
@mnapoli, are these tests sufficient? If so, I'll apply the StyleCI changes and call it good. |
The tests look good to me, testing the CLI binary would be ideal but honestly if it works, that's fine, the rest of the CLI app isn't tested so it wouldn't be fair to require that at that point. FYI StyleCI can fix your PR for you, there should be a button in the analysis to create a "Fix PR" on your branch, it's quite easy to use. |
Ok, sounds good. Side note, and a heads up, I couldn't find any documentation on the configuration you use for StyleCI. I noticed that the analysis of my branch on my account differed from the analysis on this PR. After some fiddling I figured out that the |
Apply fixes from StyleCI
Hmm, this doesn't appear to be working for me on my Win 7 machine (working on Win 10). Will troubleshoot. |
I was able to get it working by building it locally on my Win 7 machine. 🤷♂️ |
A little late but 👍 :) @wysow all good for you to merge? |
Yes @mnapoli 👍 LGTM! |
Thanks @cyberbit! |
Happy to help! Once I get home I'll work on some documentation. |
Implements #159.
This is a work in progress. I've only implemented it for the
preview
command so far. Tests are still to come.