Added Support for excluding Tables #132

Merged
merged 0 commits into from Apr 11, 2012

Conversation

Projects
None yet
3 participants
@ghost

ghost commented Apr 10, 2012

No description provided.

@stof stof and 1 other commented on an outdated diff Apr 10, 2012

lib/Doctrine/DBAL/Configuration.php
{
- $this->_attributes['filterSchemaAssetsExpression'] = $filterExpression;
+ $this->_attributes['filterSchemaAssetsExpression'] = array(
+ 'assets' => $filterExpression,
+ 'include' =>$filter
@stof

stof Apr 10, 2012

Member

missing space here. and please add the comma after $filter to avoid unnecessary changes in the diff when adding new elements

@Hounddog

Hounddog Apr 10, 2012

i am a bit confused why there should be a trailing comma without further elements. Will add it though.

@stof

stof Apr 10, 2012

Member

@Hounddog PHP allows adding a trailing comma in arrays. And this is helpful when formatting them on several lines to avoid such diffs just because of the comma:

array(
-            'assets' => $filterExpression
+            'assets' => $filterExpression,
+            'include' =>$filter
)

@stof stof and 1 other commented on an outdated diff Apr 10, 2012

lib/Doctrine/DBAL/Schema/AbstractSchemaManager.php
if ( ! $filterExpr) {
return $assetNames;
}
return array_values (
array_filter($assetNames, function ($assetName) use ($filterExpr) {
$assetName = ($assetName instanceof AbstractAsset) ? $assetName->getName() : $assetName;
- return preg_match('(' . $filterExpr . ')', $assetName);
+ if($filterExpr['include']) {
+ return preg_match('(' . $assets . ')', $assetName);
+ } else {
@stof

stof Apr 10, 2012

Member

you don't need to use else here as the if returns.
and a space is missing after the if

@Hounddog

Hounddog Apr 10, 2012

I just saw i had anyhow one more mistake which i will correct. Thank you for reviewing this allready.
so i am adding here now

return array_values (
array_filter($assetNames, function ($assetName) use ($filterExpr) {
$assetName = ($assetName instanceof AbstractAsset) ? $assetName->getName() : $assetName;
if($filterExpr['include']) {
return preg_match('(' . $filterExpr . ')', $assetName);
}
return !preg_match('(' . $filterExpr . ')', $assetName);
})
);

@stof stof commented on an outdated diff Apr 10, 2012

lib/Doctrine/DBAL/Configuration.php
*/
- public function setFilterSchemaAssetsExpression($filterExpression)
+ public function setFilterSchemaAssetsExpression($filterExpression, $filter = 'include')
@stof

stof Apr 10, 2012

Member

why is it a string ? You are using it as a boolean later

Member

stof commented Apr 10, 2012

why are you doing pull requests between 2 different repos ? This means that you add merge commits each time you do a change

@stof stof commented on an outdated diff Apr 10, 2012

lib/Doctrine/DBAL/Schema/AbstractSchemaManager.php
if ( ! $filterExpr) {
return $assetNames;
}
return array_values (
array_filter($assetNames, function ($assetName) use ($filterExpr) {
$assetName = ($assetName instanceof AbstractAsset) ? $assetName->getName() : $assetName;
- return preg_match('(' . $filterExpr . ')', $assetName);
+ if($filterExpr['include']) {
@stof

stof Apr 10, 2012

Member

the space is still missing between the if and the brace

We where not sure how long it would take till these changes would be accepted and using them at work also. so it would be beneficial if we can continue using them for our self. Will check into your suggestions. was also not happy about the string and thinking how to name the variable appropriatly

Member

stof commented Apr 10, 2012

@Hounddog but why doing the changes in your fork and then sending PRs @doyousoft's fork ? It would be easier to send the PR from the repo in which the job is done.

Btw, you should use feature branches instead of working in your master branch. It would harm your repo if a PR is rejected or if you want to create a second one before the first is merged.

Hounddog merged commit 50fb812 into doctrine:master Apr 11, 2012

i would suggest rejecting this pr for the moment... am going to put up a new one in some time... need to organize my structure here first... sorry for messing some things up :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment