Closed yurii-github closed 3 years ago
Why do you close this?
I just wanted a feedback, if it is ok I can keep PR open. I have some ideas to make tests better and drop some dependencies, because in my view this package can live without them.
by the way, builds for Laravel 5.6 are failing in master due to illuminate/database requirement (as I understand) regards
by the way, builds for Laravel 5.6 are failing in master due to illuminate/database requirement (as I understand) regards
I just wanted a feedback, if it is ok I can keep PR open. I have some ideas to make tests better and drop some dependencies, because in my view this package can live without them.
Every contribution for improvements is accepted
I just wanted a feedback, if it is ok I can keep PR open. I have some ideas to make tests better and drop some dependencies, because in my view this package can live without them.
Which dependencies do you want to drop?
by the way, builds for Laravel 5.6 are failing in master due to illuminate/database requirement (as I understand) regards
We can bump the minimum Laravel version (-> 5.7)
as sum up of my audit of the code
what i have done
latest build https://travis-ci.org/github/yurii-github/compoships/builds/738894383
i wonder what you say. only after we agree or disagree on further move/plans I continue with morph support.
see you
p.s. about dropping support up to 5.7 - for me it is vital to keep 5.6 support as I personally depend on it in one way or another. p.p.s. i will do rebase tomorrow
Sorry for the delay. I'm very busy on weekdays. Thanks for the improvements. I'll check them during the weekend and give feedback
hello sure, no problems. I have almost finished with testing setup. I have added codeclimate as code coverage, so you just need to setup your own keys on travis ci for CC_TEST_REPORTER_ID
I will add some more tests to cover my cases, and that's probably it. p.s. I would also recommend to remove Model class as trait usage is sufficient.
https://travis-ci.com/github/yurii-github/compoships/builds/195171843
I think I did around 90% of what I wanted initially. Along this journey I have found several issues too. anyway, test coverage looks like this by now.
p.s. i will squash commits tomorrow
Wow!! That's a lot of contributions. Thanks!
Can you please update the README for the steps required to run the tests?
good day sure, but give me a week or so, I have some very high work load this week, maybe I delay it till this weekend.
I have finished with my changes. I will be glad to hear your feedback on code review etc. If you find it is too big, we can split it in small PRs and I can do it that way, or whatever you think is good.
I have unresolved only 1 thing - I don't know how to forbid user to install illuminate/database 5.6 with PHP 7.3+, because it is laravel issue. maybe some note in readme is enough, I don't know
Also, there is place for improvement like test coverage, because this project is still very risky in sense of changes and tweaks it provides.
regards
I have finished with my changes. I will be glad to hear your feedback on code review etc. If you find it is too big, we can split it in small PRs and I can do it that way, or whatever you think is good.
I have unresolved only 1 thing - I don't know how to forbid user to install illuminate/database 5.6 with PHP 7.3+, because it is laravel issue. maybe some note in readme is enough, I don't know
Also, there is place for improvement like test coverage, because this project is still very risky in sense of changes and tweaks it provides.
regards
FINAL CHANGELOG
Changes
- dropped requirement for PHP version, it is enforced by illuminate/database package
- upped support for illuminate/database to 5.6+ (it requires PHP 7.1+ https://packagist.org/packages/illuminate/database#5.6.x-dev)
- dropped hard dependencies on framework and other useless packages
- removed Model class as it is not needed
- removed ComposhipsServiceProvider as it is not needed
- developer now requires to obtain PHPUnit phar manually
Fixes
- fixed several bugs for illuminate/database 5.6
- fixed several bugs for illuminate/database 5.8
Tests
- added namespaces to tests so they can be resolved with autoload
- added support for different PHP versions (7.1+)
- added TravisCI support
- added Codeclimate support
- added more tests to cover more edge cases
- other minor fixes to tests
Very nice! I'll give you feedback before next week.
All good! There are some unused variables in the tests but that's not an issue (we can clear them later).
How would you suggest to handle tests/Unit/HasManyTest:broken_Compoships_hasOneOrMany_whereInMethod__missingRelationColumn
to make it complete? We can put this warning in the doc as a workaround.
hello. thank you for the feedback
All good! There are some unused variables in the tests but that's not an issue (we can clear them later). How would you suggest to handle
tests/Unit/HasManyTest:broken_Compoships_hasOneOrMany_whereInMethod__missingRelationColumn
to make it complete? We can put this warning in the doc as a workaround.
I guess it is directly related to this issue https://github.com/topclaudy/compoships/issues/98 in my view, we should enforce selection of such fields in implicit way somewhere in with() or in some final query builder method. I can do it in new PR to target that particular issue. So warning is probably ok?
in any case, I have granted you write access to this PR branch, you can change everything you want and I will continue on master after.
regards
Merged! You can go ahead for the PR regarding https://github.com/topclaudy/compoships/issues/98
thanks.
Small question.. how do I fit code into this styleci.io ? is there any local tool to autofix it?
thanks.
Small question.. how do I fit code into this styleci.io ? is there any local tool to autofix it?
It provides a diff/patch file you can apply to the code using the git apply [DIFF_FILE]
command.
I see. thank you
some current ideas I have to improve.
I wonder what do you think about it. If you don't mind I can keep this PR open like work in progress.