Double Check Phpcs auto-fixing via phpcbf

[digisavvy]

So, if you're gonna go and do all this work to help out, you're going to become an official collaborator too, so I can assign you issues, haha. =)

What do you think of leveraging Phpcbf in the Gulp Watch task to autofix fixable issues? Also, let me know if Dev looks okay. I'm thinking of merging into master.

[desaiuditd]

Thanks :smile:

I could try this. The only hurdle I see here is whether the gulp utility that we have used for phpcs i.e., gulp-phpcs supports phpcbf or not. I'll try it out at my end. If t works then we are good.

I'm still having a thought on whether to include it in watch or not. We can add it as separate gulp task. Since many of developers would not want their code to get updated on its own. We are reporting the errors using phpcs and it's fine. On top of that, if anyone wants to solve them manually or via phpcbf is their choice. What say ?

Yup. dev looks good. I think we can merge it after solving phpcs issues. I purposefully did not solve them in PR because it could have resulted into unnecessary conflicts. I'll take a look.

[desaiuditd]

Hey @digisavvy I checked out development branch.

I think there's some problem with gulp tasks for JS & CSS. Every time I run gulp command, it changes the content of CSS files & production.js.

Is it something that I'm doing wrong or anything else ? Can you help me out here ?

[digisavvy]

I have made some changes. What is the error?

The changes I made had to do with bower primarily. The dependent scripts I was pulling in with bower I decided to enqueue instead of combining in production.js. The reason is so theme authors can easily de-queue scripts they don't want to use. So if you have any scripts in the bower_components folder you may want to remove those. Although the development branch should be current.

[digisavvy]

Just remembered, I did make a change to the jshint task. I removed the vendor directory from getting scanned by the jshint task. This is where I moved the theme dependent scripts like flexnav and modernizr to.

[digisavvy]

Do another pull; there's a lot of other commits on this branch. I'm just trying to clean up some things here.

[digisavvy]

Derp. Last comment here. I've carried over additional changes into the 1.2 branch. So pull this down and see if you still have issues. Once this is good (which seems close) I'm going to merge into the master branch

[desaiuditd]

Sure. I'll check.

[desaiuditd]

Also, one suggestion. I see lots of branches in this repo, I guess with version numbers.

You can maintain tags instead of branches for your theme versions. That way it's easier to maintain branches.

[desaiuditd]

I checked up with the new changes. All is fine. I guess, the old bower dependencies were causing the problem. It's good to go on master. Thanks :smiley:

[digisavvy]

I've merged into master and created a tag (1.2.1). Can we make sure the phpcs/phpcbf are working properly. There's an issue with validating header.php. Thx.

[desaiuditd]

:+1: That's great :smiley:

I checked out master branch for header.php. There were some issues in phpcs. I've resolved them on my local. Where should I commit them ?

[digisavvy]

Coolio! =) Just commit to master for right now. I'll create a tag from master to perform changes on soon

[desaiuditd]

Also, I checked out for phpcbf task in gulp.

The thing is we have used gulp-phpcs npm module for PHPCS task. I digged in further to see whether we can execute phpcbf with that or not. But it seems that gulp-phpcs doesn't support phpcbf at the moment. So I'm kind of stuck here for phpcbf. Though phpcs is working smoothly.

Thoughts ?

Ref :

https://github.com/JustBlackBird/gulp-phpcs

[desaiuditd]

Sure. :+1:

[digisavvy]

I came to that same conclusion, too! No worries, though. I'll come back to it eventually and re-examine.

[gaving]

FWIW I've wrapped phpcbf for gulp over here: https://github.com/gaving/gulp-phpcbf if anyone wants to check it out.