Skip to content
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

[WIP] Refactoring #222

Merged
merged 20 commits into from Apr 25, 2017
Merged

[WIP] Refactoring #222

merged 20 commits into from Apr 25, 2017

Conversation

@matej21
Copy link
Member

matej21 commented Apr 22, 2017

No description provided.

@matej21 matej21 force-pushed the matej21:refactoring branch from a890527 to f761107 Apr 24, 2017
@hrach hrach self-requested a review Apr 24, 2017
Copy link
Member

hrach left a comment

just some coding style things, otherwise one big 👍 👏

{
$key = md5(json_encode($connection->getConfig()));
$this->connection = $connection;
$this->cache = $cache->derive('mapper.' . $key);
$this->mapperCoordinator = $mapperCoordinator;

This comment has been minimized.

Copy link
@hrach

hrach Apr 24, 2017

Member

plese move the assign one line above to match the arguments :-)


class DbalMapperCoordinator
{

This comment has been minimized.

Copy link
@hrach

hrach Apr 24, 2017

Member

drop the line

@@ -0,0 +1,50 @@
<?php declare(strict_types = 1);

This comment has been minimized.

Copy link
@hrach

hrach Apr 24, 2017

Member

missing phpdoc

$this->transactionActive = false;
}
}

This comment has been minimized.

Copy link
@hrach

hrach Apr 24, 2017

Member

drop the empty line

@@ -15,7 +15,7 @@
use Nextras\Dbal\Platforms\IPlatform;
use Nextras\Orm\InvalidArgumentException;
use Nextras\Orm\InvalidStateException;
use Nextras\Orm\Mapper\IMapper;
use Nextras\Orm;

This comment has been minimized.

Copy link
@hrach

hrach Apr 24, 2017

Member

is it correctly sorted?



public function __construct(IRepository $repository, IDependencyProvider $dependencyProvider = null)
public function __construct(IRepository $repository)

This comment has been minimized.

Copy link
@hrach

hrach Apr 24, 2017

Member

empty line before consturctor

@@ -60,12 +53,6 @@ public function setRepository(IRepository $repository);
public function getRepository(): IRepository;


public function getTableName(): string;

This comment has been minimized.

Copy link
@hrach

hrach Apr 24, 2017

Member

why is this method removed?

This comment has been minimized.

Copy link
@hrach

hrach Apr 24, 2017

Member

but probably ok :)

This comment has been minimized.

Copy link
@matej21

matej21 Apr 25, 2017

Author Member

it is only used by a mapper

@matej21 matej21 force-pushed the matej21:refactoring branch from f761107 to 69bbff7 Apr 25, 2017
@matej21
Copy link
Member Author

matej21 commented Apr 25, 2017

@hrach done

@matej21 matej21 force-pushed the matej21:refactoring branch from 69bbff7 to 166d43d Apr 25, 2017
@hrach
Copy link
Member

hrach commented Apr 25, 2017

🎉
thanks!

@hrach hrach merged commit 540c261 into nextras:master Apr 25, 2017
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.07%) to 93.957%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.