-
Notifications
You must be signed in to change notification settings - Fork 29
Add Generator Factory #31
Add Generator Factory #31
Conversation
CHANGELOG.md
Outdated
@@ -6,11 +6,14 @@ All notable changes to this project will be documented in this file, in reverse | |||
|
|||
### Added | |||
|
|||
- Nothing. | |||
- Service factory `Zend\Di\Container\GeneratorFactory`, to create a |
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.
PR number and link where this change has been made.
CHANGELOG.md
Outdated
- Nothing. | ||
- Added `getOutputDirectory` to `Zend\Di\CodeGenerator\GeneratorTrait`. | ||
|
||
- Added `getNamespace()` to `Zend\Di\CodeGenerator\InjectorGenerator`. |
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.
The same here, please see CHANGELOG in another repo, for example:
https://github.com/zendframework/zend-db/blob/master/CHANGELOG.md
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.
Also for me these changes, are additions so should be in "Added" section.
docs/book/codegen.md
Outdated
@@ -14,7 +14,7 @@ based on these results. | |||
> $ composer require zendframework/zend-code | |||
> ``` | |||
|
|||
## Generating an optimized injector | |||
# Generating an optimized injector |
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.
I think it was right, we should have only one header 1st lvl in each doc.
docs/book/codegen.md
Outdated
@@ -46,3 +46,43 @@ You can also utilize `Zend\Code\Scanner` to scan your code for classes: | |||
$scanner = new DirectoryScanner(__DIR__); | |||
$generator->generate($scanner->getClassNames()); | |||
``` | |||
|
|||
# MVC and Expressive integration |
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.
It should be 2nd lvl header
src/CodeGenerator/GeneratorTrait.php
Outdated
@@ -80,4 +80,12 @@ public function setOutputDirectory(string $dir, ?int $mode = null) : self | |||
|
|||
return $this; | |||
} | |||
|
|||
/** | |||
* @return string |
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.
Doc-block comment is useless here, as function has defined return type.
What's more it is not consistent with return type (or maybe it is on purpose?)
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.
It was auto generated by ZendStudio and i forgot to remove it ...
<?php | ||
/** | ||
* @see https://github.com/zendframework/zend-di for the canonical source repository | ||
* @copyright Copyright (c) 2017 Zend Technologies USA Inc. (http://www.zend.com) |
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.
https link for zend.com
'dependencies' => [ | ||
'auto' => [ | ||
'aot' => [ | ||
'directory' => $expected |
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.
trailing comma after last element please
{ | ||
$container = $this->getMockBuilder(ContainerInterface::class)->getMockForAbstractClass(); | ||
$container->method('has')->willReturnCallback(function ($type) { | ||
return ($type == ConfigInterface::class); |
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.
parenthesis around the expression are useless
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.
I find this more expressive and readable, but this is personal preference. I will remove it.
|
||
public function testFactoryUsesDiConfigContainer(): void | ||
{ | ||
$container = $this->getMockBuilder(ContainerInterface::class)->getMockForAbstractClass(); |
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.
It's ok, but probably you'll find easier to use prophecy (it is required by phpunit, and we are using it in other repos)
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.
Thanks for the hint, I'll keep this in mind the next time
$factory->create($container); | ||
} | ||
|
||
public function testSetsOutputDirectoryFromConfig() |
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.
return type void for consistency, also in other test methods in that file
->method('create') | ||
->with($container); | ||
|
||
$mock($container); |
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.
Here we should assign the result and do some assertions on it.
CHANGELOG.md
Outdated
|
||
- Added `getNamespace()` to `Zend\Di\CodeGenerator\InjectorGenerator`. | ||
- #31 Added `getNamespace()` to `Zend\Di\CodeGenerator\InjectorGenerator`. |
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.
still missing links to the PR, and these changes should be moved to "Added" section, as these are additions
{ | ||
$container = $this->getMockBuilder(ContainerInterface::class)->getMockForAbstractClass(); | ||
$container->method('has')->willReturnCallback(function ($type) { | ||
return ($type == ConfigInterface::class); | ||
return $type == ConfigInterface::class; |
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.
Didn't note it previously, I would use here ===
as we started using strict types
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.
I've pushed a few commits that provide edits to the changelog and documentation; otherwise, looks great!
When you are ready to release, be sure to remove the 3.0.1 stub in the CHANGELOG, as well as to set the date for the 3.1.0 release in it. Also, after pushing the tag, edit the release on github, name it "zend-di 3.1.0", and copy-paste the 3.1.0 changelog entry. These actions will trigger a webhook that will tweet the release, as well as update the master ZF release feed.
Thanks, @tux-rampage !
Great thank you very much |
This PR provides service factories for the AoT code generators, to minimize repetetive Tasks when using AoT.
The factory should will the
config
service to obtain information such as target namespace and output directory.As Stated in #30, component consumers will then be able to retrieve the generator from the IoC container:
develop
branch, and submit against that branch.CHANGELOG.md
entry for the new feature.