Feature/102

[acelaya]

@samsonasik Just finished the Navigation module. Let me know what you think.

[acelaya]

I'm not sure why the build is failing. Apparently there is a CS error in the module.config.php file in LearnZF2Pagination, but I can't get it fixed in my local machine. Any ideas?

[samsonasik]

@acelaya first, you need to do rebase ( or pull with fast-forward ) instead of merge master into your branch.

git checkout master // make sure you're comes from master
git pull upstream master // make sure you're uptodate ( or git pull git://github.com/sitrunlab/LearnZF2.git master )
git checkout -b new_module_nav master // create new module based on master

if branch new_module_nav exists, then you just need to rebase againts master :

git checkout new_module_nav
git rebase master

Rebasing will fix overlapped commits.

And before do push, run :

composer self-update
composer update
vendor/bin/php-cs-fixer fix module // this should use latest php-cs-fixer

Now, check git status :

git status

if there is a change, do latest commit and re-try git push --force.

git commit -m 'message' -a
git push --force origin new_module_nav

This should fix the issue above. If the problem persist, then I think you need to cherry-pick the commits to new branch and re-rebase against latest master.

[samsonasik]

btw, I added readme git on https://github.com/sitrunlab/LearnZF2/blob/master/Work-with-Git.md for that.

[samsonasik]

more notes : 1 need codeception acceptance test 2 need LICENSE file inside module 3 need docblock license header in files.

[samsonasik]

overall, it's great, need some fixes above, make sure test still 100%, and we can do more review and I can test locally. Thank you ;)

[acelaya]

Prefect. I will fix the branch. I don't usually like to rebase branches that I've already pushed, because someone else could be using it, but this is not the case, because it's in my own fork. I should have rebased master, sorry. I merged it without really thinking on it.

The case of the code sniffer errors is weird. The first time the build failed I run the cs fixer and it found the 4 errors I saw in the build output. I committed and pushed, but the build failed with a file in the pagination module, and my local instance wasn't finding it. The problem probably was that I had an outdated version of the cs fixer.

I'll fix those two things and everything else you mentioned, this afternoon. ;-)

Alejandro Celaya Alastrué www.alejandrocelaya.com El 2/3/2015 1:08, "Abdul Malik Ikhsan" notifications@github.com escribió:

overall, it's great, need some fixes above, make sure test still 100%, and we can do more review and I can test locally. Thank you ;)

— Reply to this email directly or view it on GitHub https://github.com/sitrunlab/LearnZF2/pull/117#issuecomment-76640029.

[samsonasik]

great, thank you ;)

Warm regards,

Abdul Malik Ikhsan

Pada 2 Mar 2015, pukul 14.11, Alejandro Celaya notifications@github.com menulis:

Prefect. I will fix the branch. I don't usually like to rebase branches that I've already pushed, because someone else could be using it, but this is not the case, because it's in my own fork. I should have rebased master, sorry. I merged it without really thinking on it.

The case of the code sniffer errors is weird. The first time the build failed I run the cs fixer and it found the 4 errors I saw in the build output. I committed and pushed, but the build failed with a file in the pagination module, and my local instance wasn't finding it. The problem probably was that I had an outdated version of the cs fixer.

I'll fix those two things and everything else you mentioned, this afternoon. ;-)

Alejandro Celaya Alastrué www.alejandrocelaya.com El 2/3/2015 1:08, "Abdul Malik Ikhsan" notifications@github.com escribió:

overall, it's great, need some fixes above, make sure test still 100%, and we can do more review and I can test locally. Thank you ;)

— Reply to this email directly or view it on GitHub https://github.com/sitrunlab/LearnZF2/pull/117#issuecomment-76640029.

— Reply to this email directly or view it on GitHub.

[acelaya]

Sorry @samsonasik, This week I'm busier than I thought. This Thursday is bank holiday here. I'll try to review this by then.

[samsonasik]

It's ok, take your time ;). Thank you ;)

Warm regards,

Abdul Malik Ikhsan

Pada 4 Mar 2015, pukul 01.03, Alejandro Celaya notifications@github.com menulis:

Sorry @samsonasik, This week I'm busier than I thought. This Thursday is bank holiday here. I'll try to review this by then.

— Reply to this email directly or view it on GitHub.

[acelaya]

Just fixed everything you said, but code sniffer fixer keeps throwing more and more errors. Indeed I run it locally over the "modules" directory and it changed a lot of files. I'm using php-cs-fixer v1.5, the same that is being installed at build time. Have you tried this version? In my local environment I run vendor/bin/php-cs-fixer fix module -v --dry-run --config-file=.php_cs. Is there anything I'm doing wrong?

[samsonasik]

hm.., codesniffer and php-cs-fixer have different criteria for fixing cs iirc. I will pull it locally and try it, if everything ok, then I will fix the cs during merge. thank you ;)

[acelaya]

Ok, thanks Abdul. I was starting to go crazy ;-)

Alejandro Celaya Alastrué www.alejandrocelaya.com

Merged #117 https://github.com/sitrunlab/LearnZF2/pull/117.

— Reply to this email directly or view it on GitHub https://github.com/sitrunlab/LearnZF2/pull/117#event-246662465.

[samsonasik]

You're welcome. Everything ok, I've fixed cs during merge and update .php_cs config. travis should be green in a few minutes. Thank you ;)