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

Scoring refactoring #134

Closed
wants to merge 27 commits into
from

Conversation

Projects
None yet
3 participants
Contributor

Palleas commented Feb 10, 2012

(This should be easier to merge when this PR is merged)

This is a new "approach" for the scoring algorithm, using the event dispatcher to make it more easy to maintain and less coupled to the whole update process.

Let me know what you think! :)

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

...ndle/KnpBundlesBundle/EventDispatcher/BundleEvent.php
@@ -0,0 +1,35 @@
+<?php
+
+namespace Knp\Bundle\KnpBundlesBundle\EventDispatcher;
@stof

stof Feb 10, 2012

Contributor

it should be Event to be consistent with Symfony

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

...ndle/KnpBundlesBundle/EventDispatcher/BundleEvent.php
@@ -0,0 +1,35 @@
+<?php
+
+namespace Knp\Bundle\KnpBundlesBundle\EventDispatcher;
+
+use Knp\Bundle\KnpBundlesBundle\Entity\Bundle;
+use Symfony\Component\EventDispatcher\Event;
+/**
@stof

stof Feb 10, 2012

Contributor

missing newline after the use statement

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

src/Knp/Bundle/KnpBundlesBundle/Github/Repo.php
@@ -3,9 +3,12 @@
namespace Knp\Bundle\KnpBundlesBundle\Github;
use Symfony\Component\Console\Output\OutputInterface;
+use Symfony\Component\EventDispatcher\EventDispatcher;
@stof

stof Feb 10, 2012

Contributor

please use the interface when typehinting instead of the implementation

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

.../Bundle/KnpBundlesBundle/Scoring/ActivityListener.php
@@ -0,0 +1,20 @@
+<?php
+
+namespace Knp\Bundle\KnpBundlesBundle\Scoring;
@stof

stof Feb 10, 2012

Contributor

you should put the listeners in the EventListener namespace to follow the convention (you can use a Scoring subnamespace if you want to keep things organized when adding other listeners)

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

.../Bundle/KnpBundlesBundle/Scoring/ActivityListener.php
@@ -0,0 +1,20 @@
+<?php
+
+namespace Knp\Bundle\KnpBundlesBundle\Scoring;
+
+use Knp\Bundle\KnpBundlesBundle\Entity\Bundle;
+
+/**
+*
+*/
+class ActivityListener extends ScoringListener
+{
+
+ public function updateScore(Bundle $bundle)
@stof

stof Feb 10, 2012

Contributor

you have extra empty lines at the beginning and the end of the class (same in next listeners)

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

src/Knp/Bundle/KnpBundlesBundle/Entity/Score.php
+ protected $travisbuild;
+
+ /**
+ * Score detail based on the bundle being installable using composer
+ *
+ * @ORM\Column(type="integer")
+ */
+ protected $composer;
+
+ /**
+ * Score detail based on the number of people who recommended this bundle
+ *
+ * @ORM\Column(type="integer")
+ */
+ protected $recommenders;
+
@stof

stof Feb 10, 2012

Contributor

this seems dirty. I don't think you should store all details in the DB. What if you want to update your scoring algorithm ? Will you change the DB schema each time ?

@Palleas

Palleas Feb 10, 2012

Contributor

Hum, you're right, didn't think about that...
Any idea? Storing only latest score detail on a serialized array?

@stof

stof Feb 10, 2012

Contributor

well, do you need to store it ?

@Palleas

Palleas Feb 10, 2012

Contributor

I just don't want it to be recalculated each time.

Contributor

stof commented Feb 11, 2012

@Palleas you should update your PR so that github sees that the first commits are merged now (it won't change the diff if the PR is untouched)

Contributor

Palleas commented Feb 11, 2012

@stof I think I've fixed everything now, thanks for your review :)

@stof stof and 1 other commented on an outdated diff Feb 11, 2012

...dlesBundle/EventListener/Scoring/ActivityListener.php
@@ -0,0 +1,21 @@
+<?php
+
+namespace Knp\Bundle\KnpBundlesBundle\EventListener\Scoring;
+
+use Knp\Bundle\KnpBundlesBundle\Entity\Bundle;
+
+/**
+*
+*/
@stof

stof Feb 11, 2012

Contributor

broken indentation

@stof

stof Feb 11, 2012

Contributor

and same for all other created by copy-paste

@Palleas

Palleas Feb 12, 2012

Contributor

Fixed, thanks!

@stof stof commented on the diff Feb 12, 2012

src/Knp/Bundle/KnpBundlesBundle/Event/BundleEvent.php
@@ -0,0 +1,33 @@
+<?php
+
+namespace Knp\Bundle\KnpBundlesBundle\Event;
+
+use Knp\Bundle\KnpBundlesBundle\Entity\Bundle;
+use Symfony\Component\EventDispatcher\Event;
+
+class BundleEvent extends Event
+{
+
@stof

stof Feb 12, 2012

Contributor

you have an extra line here

@stof stof commented on the diff Feb 12, 2012

...npBundlesBundle/Resources/views/Bundle/show.html.twig
<div class="box">
<h3 id="bundle-score-details">{% trans %}bundles.show.score.title{% endtrans %} ( <a href="{{ path('scoring') }}">{% trans %}bundles.show.score.about{% endtrans %}</a> )</h3>
<ul class="bundle-score-details">
- <li>{% trans with {'%score%': score.followers } %}bundles.score.followers{% endtrans %}</li>
- <li>{% trans with {'%score%': score.activity } %}bundles.score.activity{% endtrans %}</li>
- <li>{% trans with {'%score%': score.readme } %}bundles.score.readme{% endtrans %}</li>
- <li>{% trans with {'%score%': score.travisci } %}bundles.score.travisci{% endtrans %}</li>
- <li>{% trans with {'%score%': score.travisbuild } %}bundles.score.travisbuild{% endtrans %}</li>
- <li>{% trans with {'%score%': score.composer } %}bundles.score.composer{% endtrans %}</li>
- <li>{% trans with {'%score%': score.recommenders } %}bundles.score.recommenders{% endtrans %}</li>
+ <li>{% trans with {'%score%': score_details.followers } %}bundles.score.followers{% endtrans %}</li>
@stof

stof Feb 12, 2012

Contributor

undefined variable score_details

@stof stof commented on an outdated diff Feb 12, 2012

...undle/Tests/EventListener/Scoring/GlobalScoreTest.php
@@ -0,0 +1,54 @@
+<?php
+
+namespace Knp\Bundle\KnpBundlesBundle\Tests\EventListener\Scoring;
+
+use Knp\Bundle\KnpBundlesBundle\Entity\Bundle;
+use Knp\Bundle\KnpBundlesBundle\Entity\User;
+use Knp\Bundle\KnpBundlesBundle\Event\BundleEvent;
+
+use Symfony\Component\EventDispatcher\EventDispatcher;
+
+/**
+*
+*/
@stof

stof Feb 12, 2012

Contributor

wrong indentation (and useless empty phpdoc)

@stof stof commented on an outdated diff Feb 12, 2012

...ests/EventListener/Scoring/KnpBundlesListenerTest.php
@@ -0,0 +1,31 @@
+<?php
+
+namespace Knp\Bundle\KnpBundlesBundle\Tests\EventListener\Scoring;
+
+use Knp\Bundle\KnpBundlesBundle\Entity\Bundle;
+use Knp\Bundle\KnpBundlesBundle\Entity\User;
+use Knp\Bundle\KnpBundlesBundle\EventListener\Scoring\KnpBundlesListener;
+
+/**
+*
+*/
@stof

stof Feb 12, 2012

Contributor

same here

@stof stof commented on the diff Feb 12, 2012

...ndlesBundle/EventListener/Scoring/ScoringListener.php
+ *
+ * @param Knp\Bundle\KnpBundlesBundle\EventDispatcher\BundleEvent
+ */
+ public function onScoreUpdate(BundleEvent $event)
+ {
+ $this->updateScore($event->getBundle());
+ }
+
+ /**
+ * Add details to bundle's score
+ *
+ * @param Knp\Bundle\KnpBundlesBundle\Entity\Bundle
+ */
+ abstract public function updateScore(Bundle $bundle);
+
+}
@stof

stof Feb 12, 2012

Contributor

extra empty lines at the beginning and the end of the class

@stof stof and 2 others commented on an outdated diff Feb 12, 2012

src/Knp/Bundle/KnpBundlesBundle/Entity/Bundle.php
- return $score;
+ public function setScoreDetails(array $details)
+ {
+ $this->scoreDetails = serialize($details);
@stof

stof Feb 12, 2012

Contributor

don't serialize it. Doctrine does it inside the ArrayType, and doing it here means your class is broken as the property contains a string instead of an array

@ubermuda

ubermuda Mar 18, 2012

Contributor

ping @Palleas, you should really fix that I guess?

@Palleas

Palleas Mar 19, 2012

Contributor

Done.

@ubermuda ubermuda closed this Mar 23, 2012

Contributor

ubermuda commented Mar 23, 2012

merged

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