search

OpenBuildings / paypal

PayPal SDK for ExpressCheckout and AdaptivePayments.
Other
29 stars 24 forks source link

Cleaned source code #7

Closed marcw closed 11 years ago

marcw commented 11 years ago

This is a clean of the source code to respect PSR standards. Tests are still passing.

hkdobrev commented 11 years ago

Composer.lock should not be commited for libraries

I disagree. So does the Composer authors.

From http://getcomposer.org/doc/02-libraries.md#lock-file:

For your library you may commit the composer.lock file if you want to. This can help your team to always test against the same dependency versions. However, this lock file will not have any effect on other projects that depend on it. It only has an effect on the main project.

If a team is working on a library it is useful to commit Composer.lock as much as it is for a project.

hkdobrev commented 11 years ago

Tests are not part of the library executable source code. So they are in a different folder with a separate autoloading. You could use namespaces and so on with tests, but they should be completely separate. Users of the library who don't know anything about tests could use only the src folder. Autoloading with Composer classmaps should happen only with the source, not with the tests.

hkdobrev commented 11 years ago

You have used the PHP CS which is nice. However I don't approve the PEAR standard for code style for this project. I have obviously missed that, but please keep the same coding style. Which is similar to http://kohanaframework.org/3.3/guide/kohana/conventions.

Some changes I see are that you have moved the opening class braces on the next line and the braces for operators on the same line.

hkdobrev commented 11 years ago

I like some things like: https://github.com/marcw/paypal/commit/31a5c2934605a1c57143c9d86f74b8849a51a7ec#L4R48, but this should be in a separate PR.

Even if a PR is really, really good when it is so big you cannot comprehend all changes at once and something would not get reviewed. I would love to see the meaningful parts of this PR separated into different ones.

marcw commented 11 years ago

The composer.json do not really list important dependencies, that's why I removed the composer.lock file. For the CS and the tests files, I was following the Symfony2 way here but it doesn't really matter. It would have been nice to state which CS you were using in the contributing guidelines.

Feel free to cherry pick any commit you think is useful.