-
Notifications
You must be signed in to change notification settings - Fork 0
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
Blueprint building #6
base: master
Are you sure you want to change the base?
Conversation
Improve travis build
Parameter naming strategy for blueprint builder -> todo |
$value = $this->prepareData($value); | ||
} | ||
|
||
if (!is_int($key)) { |
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.
Key can any, not only int or 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.
The key can either be an integer or a string. The value can be of any type.
Other types are casted underlying to string or int.
{ | ||
$pattern = $this->generator->create($class); | ||
|
||
return eval($pattern); |
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.
You should check this value before return to avoid typeerror
private $nullable; | ||
|
||
/** @param mixed $defaultValue */ | ||
public function __construct(string $type, string $name, bool $nullable = false, $defaultValue = null) |
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.
IMO it's better to expects all parameters
return $this->name; | ||
} | ||
|
||
public function withDefaultValue(): bool |
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.
Looks like method which changes immutable object. hasDefaultValue?
|
||
public function withDefaultValue(): bool | ||
{ | ||
return null !== $this->defaultValue; |
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.
Null it's still value
return $this->serializeObjectListNode($node); | ||
} | ||
|
||
throw new BuildingError(); |
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.
Missing details about this error.
{ | ||
try { | ||
return $this->store[$class]; | ||
} catch (Throwable $exception) { |
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.
Why?
src/Builder/Reflection.php
Outdated
$constructorMethod = $class->getConstructor(); | ||
$parameters = []; | ||
|
||
if (null !== $constructorMethod) { | ||
$parameters = iterator_to_array($this->collect($constructorMethod, $data)); | ||
/** @var \Traversable $iterator */ | ||
$iterator = $this->collect($constructorMethod, $data); |
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.
Looks like duplication
'int', | ||
'float', | ||
'double', | ||
'mixed', |
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.
Mixed === scalar?
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.
In context of this private function, it is. But I just didn't find better name so I'm open for alternatives name.
throw new BuildingError('Can not resolve namespace for class ' . $className); | ||
} | ||
|
||
private function endsWith(string $haystack, string $needle): bool |
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.
Maybe move it to function or trait
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.
Not now, because I will refactor Builder\Reflection
to use PhpDocParser\PhpStan
{ | ||
public static function setUpBeforeClass(): void | ||
{ | ||
static::$builder = new Reflection(new SnakeCase()); |
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.
Why static?
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.
Because Builders are stateless and no needs to recreate in every test case.
use PHPUnit\Framework\TestCase; | ||
use RstGroup\ObjectBuilder\Builder\Blueprint\Factory\CodeGenerator\PatternStore\Filesystem; | ||
|
||
class FilesystemTest extends TestCase |
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.
There are libs to mock file system
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, but is it good cause to use his? In integration test I don't want mock object of my integration - filesystem.
No description provided.