search

bakkerij / notifier

Notifications plugin for CakePHP 3.x
MIT License
59 stars 38 forks source link

Allow Notifier component to limit amount of notifications to return. #8

Open akkaweb opened 8 years ago

akkaweb commented 8 years ago

Changes to the method getNotifications() to accept a 3rd paremeter $limit = null and make the necessary code changes to accept limit. README.md also changed to reflect Component changes.

coveralls commented 8 years ago

Coverage Status

Coverage decreased (-1.07%) to 79.851% when pulling 413dd9ecff40dafbd87f577e48cf606c0980081e on akkaweb:master into 80bb26c63d81f3b7caa80f04680feade8b344688 on cakemanager:master.

bobmulder commented 8 years ago

Hi @akkaweb,

First of all, thank you for the PR! However, I have some suggestions for you.

1) As you see Travis failed. The reason you can see why: here. You didn't handle any code-standard. Thats why the file list is f*cked up, so I can't see that clear what code you added/changed. If you're not familiar with code standards let me know.

2) No tests are added for this functionality. Thats why coveralls came in to say that the coverage decreased. Please add tests to see if the method works without limit, and with multiple limits.

If you have questions, feel free to chat me!

Looking forward to your changes! Good luck!

akkaweb commented 8 years ago

My apologies about that. I mostly develop for Drupal and my IDE is setup to work with their coding standards. I will fix and resubmit.

coveralls commented 8 years ago

Coverage Status

Coverage decreased (-1.07%) to 79.851% when pulling 1d447dc972a348993bdc764e3d884c80cccae2e9 on akkaweb:master into 80bb26c63d81f3b7caa80f04680feade8b344688 on cakemanager:master.

akkaweb commented 8 years ago

I updated the code to comply with CakePHP standards. I will watch Travis to see if it allows it to pass now. Test cases have been added now.

coveralls commented 8 years ago

Coverage Status

Coverage increased (+0.4%) to 81.343% when pulling 3089569cd9c51bb92e07a65efe552bf7d499440e on akkaweb:master into 80bb26c63d81f3b7caa80f04680feade8b344688 on cakemanager:master.

bobmulder commented 8 years ago

@akkaweb Thank you! Good work! I added some comments you should look at. Also the code-coverage gave one error: https://travis-ci.org/cakemanager/cakephp-notifier/jobs/118129153#L278. If you can fix these, it will be okay!

As I see, you added a tirth parameter the the method, which is good enough. But I was tinking, to be prepared of the future, we could make the tirth parameter an array, with the option limit. What do you think about that?

coveralls commented 8 years ago

Coverage Status

Coverage increased (+0.4%) to 81.343% when pulling 15f325cb6bf35234166f7287f048c73f2575b436 on akkaweb:master into 80bb26c63d81f3b7caa80f04680feade8b344688 on cakemanager:master.

akkaweb commented 8 years ago

I totally agree about the 3rd parameter being an array. Thanks for catching that! I put that change into place, changed test cases to reflect that update and I also updated code-coverage to include the 3rd option. Hopefully all the tests will now pass. If any issues or if you would like something changed let me know. Thanks

coveralls commented 8 years ago

Coverage Status

Coverage increased (+0.4%) to 81.343% when pulling 15f325cb6bf35234166f7287f048c73f2575b436 on akkaweb:master into 80bb26c63d81f3b7caa80f04680feade8b344688 on cakemanager:master.

coveralls commented 8 years ago

Coverage Status

Coverage increased (+0.4%) to 81.343% when pulling 15f325cb6bf35234166f7287f048c73f2575b436 on akkaweb:master into 80bb26c63d81f3b7caa80f04680feade8b344688 on cakemanager:master.

coveralls commented 8 years ago

Coverage Status

Coverage increased (+0.4%) to 81.343% when pulling dc0b24348c61ac5a23a2872973699ce3c42fd7f8 on akkaweb:master into 80bb26c63d81f3b7caa80f04680feade8b344688 on cakemanager:master.

coveralls commented 8 years ago

Coverage Status

Coverage increased (+0.4%) to 81.343% when pulling 181ad20aa57fc51554496b054e8e088a091294bd on akkaweb:master into 80bb26c63d81f3b7caa80f04680feade8b344688 on cakemanager:master.

bobmulder commented 8 years ago

Sorry for leaving this behind @akkaweb. Lets finish this :)

I see you did 9 commits.

I would like to see the commits squased (read more at http://gitready.com/advanced/2009/02/10/squashing-commits-with-rebase.html and http://couscous.io/contributing.html)

Greetz,

Bob

akkaweb commented 8 years ago

@bobmulder Sure! I will give it a go soon! Thanks!