search

event-centric / EventCentric.Core

Event Sourcing and CQRS in PHP
MIT License
126 stars 9 forks source link

Various clean ups #7

Closed cordoval closed 10 years ago

cordoval commented 10 years ago

Sent with Gush

cordoval commented 10 years ago

first time i see a test marked as risky:

~ php bin/phpunit -c phpunit.xml tests --debug                                    Luiss-MacBook-Pro-3 [3:13:15]
PHPUnit 4.1.4 by Sebastian Bergmann.

Configuration read from /Users/cordoval/Sites/ddd/deps/event-centric/EventCentric.Core/phpunit.xml

................................R....

Time: 198 ms, Memory: 7.00Mb

OK, but incomplete, skipped, or risky tests!
Tests: 37, Assertions: 82, Risky: 1.
mathiasverraes commented 10 years ago

Thanks Luis. This PR breaks the build. It's also a mess, lots of different things mixed within the same PR, and even within the same commit. Please send 1 PR per problem. If you can't state a problem, don't send a PR.

Commits like this do not solve any problem:

-final class ConventionalCommandHandlingTest extends \PHPUnit_Framework_TestCase
+final class ConventionalCommandHandlingTest extends PHPUnit_Framework_TestCase

Closing.

cordoval commented 10 years ago

the problem i see is consistency. It is ok though, i don't send this PRs just to contribute but also to read and review code and provide some feedback. I also noticed you set your phpstorm to template for class to display final by default which is interesting, so i learn from that. However it also has some spaces at the end of the classes that is just weird.

Also the break on the build is only on hhvm which is a bit weird. https://travis-ci.org/event-centric/EventCentric.Core/jobs/31309611 . So in reality the dependency can be lowered without problems.

mathiasverraes commented 10 years ago

HHVM support is essential.

"Consistency" is not a problem statement, it's a potential solution to some other (perceived) problem. "High cognitive load when reading the code" can be a problem, and that problem can partially be solved by increasing consistency. However, the example above, or the number of spaces at the end of a class, have zero effect on readability.

cordoval commented 10 years ago

yes that "effect on readability" is somewhat subjective and i get you too. So no worries. To me is good to have good cs and "consistency" as in the PR. Some of my neuroscience :blush: reasoning but that is too long to explain. Is cool.