Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Improvement to develop behaviors #535

Merged
merged 1 commit into from Mar 3, 2014

Conversation

Projects
None yet
5 participants
Contributor

gossi commented Feb 6, 2014

This is an improvement for behavior devs. When developing a behavior, there is no composer.lock file containing the information about the behavior, instead these information are stored in the composer.json file. This PR also searches in the composer.json. This is extremly handy when writing tests, which involves running a propel build. These builds so far can't find the behavior which is currently worked on. This is fixed with this PR. Tests included.

@willdurand willdurand and 1 other commented on an outdated diff Feb 6, 2014

src/Propel/Generator/Util/BehaviorLocator.php
@@ -32,7 +32,8 @@ class BehaviorLocator
/**
* Creates the composer finder
*
- * @param GeneratorConfigInterface $config build config
+ * @param GeneratorConfigInterface $config
+ * build config
@gossi

gossi Feb 6, 2014

Contributor

true, don't trust your code formatter!

Owner

willdurand commented Feb 6, 2014

👍

Contributor

gossi commented Feb 6, 2014

I cleaned up, what my formatter changed improperly and squashed. Tests will turn green. Ready to merge then, I think.

@staabm staabm commented on the diff Feb 7, 2014

src/Propel/Generator/Util/BehaviorLocator.php
@@ -68,6 +68,26 @@ private function findComposerLock()
return null;
}
+
+ /**
+ * Searches the composer.lock file
@staabm

staabm Feb 7, 2014

Member

do we really need this 2 shortcut methods... I think they don't get us much ...

@gossi

gossi Feb 7, 2014

Contributor

Yes, I think so. At the moment all three methods are private. However findComposerFile will ever stay private, while the others may change visibility. So, it's up to the BehaviorLocator to provide access to which files can be found, that's my rational behind it.

@staabm

staabm Feb 16, 2014

Member

Still don't like this shortcut methods

Contributor

gossi commented Feb 16, 2014

What about this PR? Can this be merged?

@staabm staabm and 1 other commented on an outdated diff Feb 16, 2014

src/Propel/Generator/Util/BehaviorLocator.php
}
}
}
return $behaviors;
}
+
+ /**
+ * Reads the propel behavior data from a given composer package
+ *
+ * @param array $package
+ * @throws BuildException
+ * @return array behavior data
+ */
+ private function loadBehavior($package)
+ {
+ if (array_key_exists('type', $package) && $package['type'] == self::BEHAVIOR_PACKAGE_TYPE) {
@staabm

staabm Feb 16, 2014

Member

Shouldn't we use isset instead of array_key_exists in this method for performance reasons?

@gossi

gossi Feb 17, 2014

Contributor

I once decided for myself to use array_key_exists, because I was working with arrays. Do you have any pros/cons for each?

@staabm

staabm Feb 17, 2014

Member

isset is much faster. the diff of those 2 is, that array_key_exists return true when the key is null but isset checks for existance+non-null

@gossi

gossi Feb 17, 2014

Contributor

Ah, thanks. I will change to isset then.

Member

staabm commented Feb 16, 2014

Despite the nits, looks good to me,

Member

robin850 commented Feb 16, 2014

This is a nitpick too but I think that you should use 4 spaces instead of tabs. This will make it easier to trace history with blames since one could run the CS fixer and override your lines.

Contributor

gossi commented Feb 17, 2014

Ok, changed to isset and changed all tabs to spaces and of course squashed. Thanks for your feedback.

Owner

marcj commented Mar 3, 2014

Good job @gossi, thanks!

@marcj marcj added a commit that referenced this pull request Mar 3, 2014

@marcj marcj Merge pull request #535 from gossi/composer-behavior-dev
Improvement to develop behaviors
e8a1d47

@marcj marcj merged commit e8a1d47 into propelorm:master Mar 3, 2014

1 check passed

default The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment