Closed RyanWChild closed 6 years ago
Hi @RyanWChild Thank you so much for your effort, although I'm not sure why we need to change the naming convention if we are in the midst of adding missing unit tests to the project. By making these syntactic modifications you've made my life harder to review these changes, So I will take my time and revert back to you.
Again, thank you for the effort you've made !
cheers.
Hi @RyanWChild
These are my findings regarding your pull request:
For those two points:
I'm not sure what code conventions have to do with writing missing unit testing, it's a matter of preference, and it doesn't related to the pull request purpose.
For this Dependency I think as a Third-party library, it's good to maintain the least working dependencies, and not to force the consumer to upgrade to the latest version if the library is just fine with the least supported version.
For the previous reasons, I will Not merge the pull request, please don't take it personally, I admire what you've did, but while you are contributing in OSS world, please don't do more than what is intended in the original pull request, or it will be hard for the maintainer to give up on all these changes just to merge the PR.
I'm happy to merge you're PR again if you revert your unnecessary changes to the code.
Congrats, I will merge now !
Hi @AliBazzi!
I believe I've addressed all of your comments. Thank you for reviewing (especially on the weekend). Cheers!
Congrats on your first PR, if this the only account you use :D
Thanks for your contribution again !
Goal
Add tests for RedisCache and PersistedGrantStore
Details
_name
syntaxthis
as they were no longer needed after rename