Feature/additional enhancements #7
Conversation
| public function searchPackage(string $package) | ||
| { | ||
| $data = file_get_contents($this->buildSearchUrl($package)); | ||
| $data = json_decode($data, true); |
There was a problem hiding this comment.
should we check $data is valid? something like json_last_error() === JSON_ERROR_NONE ? array_map(...) : []
There was a problem hiding this comment.
Nice catch, implement this and for the getVersionsPackage as well
| /** | ||
| * @param array $options | ||
| */ | ||
| public function setConfig(array $options) |
There was a problem hiding this comment.
should we rename this method, we are saving/storing the config, setConfig implies you would be doing something like $this->config = $options
src/Skeletor/App/Config/skeletor.yml
Outdated
| @@ -1,11 +1,11 @@ | |||
| config: | |||
| name: Skeletor CLI | |||
| name: 'Skeletor CLI' | |||
There was a problem hiding this comment.
No need for quotes since there is no special characters in the string
There was a problem hiding this comment.
Hmm, i already couldn't remember changing this. But what happens, if you add a package and the storeConfig method runs. It exactly puts quotes around the string.
| $this->makePackageClass($packageInfo); | ||
| } | ||
|
|
||
| protected function setupCommand() |
There was a problem hiding this comment.
It seems that this method is very similar to the one implemented in the other commands, should we maybe create an abstract SkeletorCommand and put in the there shared methods and make the actual commands extend from it instead of the Symfony Command
| */ | ||
| protected function buildPackageInfo(array $packageInfo) | ||
| { | ||
| $packageName = preg_replace('/\//', ' ', $packageInfo['slug']); |
There was a problem hiding this comment.
We can group these two regular expressions, something like preg_replace('//|-/', ' ', $packageInfo['slug']);
There was a problem hiding this comment.
Nice, still learning regex 😃
|
|
||
| //Replace stub dummy data | ||
| foreach ($packageInfo as $key => $info) | ||
| { |
There was a problem hiding this comment.
following our standard, { on the same line as foreach
| public function specifyPackage($packages) | ||
| { | ||
| $packageQuestion = $this->cli->radio('Choose your packages:', $packages); | ||
| return $packageQuestion->prompt(); |
There was a problem hiding this comment.
blank line before a return
| $this->setName("Behat Behat"); | ||
| } | ||
|
|
||
| public function configure(Framework $activeFramework) |
There was a problem hiding this comment.
empty method, should we delete it?
There was a problem hiding this comment.
True, what is the best practice. Because we define configure on our packageinterface it must be defined in the child. But like you said, its probably not always going to be used.
Should i remove it from the interface?
There was a problem hiding this comment.
It case the best practise would be to have to interfaces, PackageInterface and ConfigurablePackageInterface that extends from PackageInterface. So you can pick the one you need for each case.
src/Skeletor/App/Config/skeletor.yml
Outdated
| @@ -1,11 +1,11 @@ | |||
| config: | |||
| name: Skeletor CLI | |||
| name: 'Skeletor CLI' | |||
| version: 1.0.0 | |||
There was a problem hiding this comment.
I wonder if there is a nice way to get the version number from the git tag automatically?
| @@ -42,7 +42,7 @@ protected function execute(InputInterface $input, OutputInterface $output) | |||
| $this->setupCommand(); | |||
|
|
|||
| //Start process in the background | |||
| $process = new Process('php skeletor package:show --no-ansi'); | |||
| $process = new Process('skeletor package:show --no-ansi'); | |||
There was a problem hiding this comment.
Is it right that this is being changed back? This was a bugfix you did last time to sort the issue with the packagist API, right?
There was a problem hiding this comment.
Yes, this is a separate/new bug. So on composer global, the packages aren't fetched because it cant find the command. When removing the php it works, because it can find the command again.
Added
skeletor package:addso developers can easily add new packages.Changed the class naming (packages) to the complete slug, so there will be no conflicts.