search

cpliakas / git-wrapper

A PHP wrapper around the Git command line utility.
MIT License
505 stars 68 forks source link

Add some CS linters and fixers #143

Closed soullivaneuh closed 6 years ago

soullivaneuh commented 6 years ago

We should setup some tools to maintain a certain coding style quality.

PHP-CS-Fixer and maybe Yamllint would be a good start.

We may a both CLI and SAAS service support with the tool I made: https://flintci.io

Would you like me to setup it on a PR and make a try?

Regards.

EDIT: I didn't see you have this: https://github.com/cpliakas/git-wrapper/blob/master/easy-coding-standard.neon

I don't know if this can be complementary.

TomasVotruba commented 6 years ago

Nice idea!

CS is already covered by ECS, as you reffered (it includes both PHP_CodeSniffer and PHP CS Fixer): https://github.com/cpliakas/git-wrapper/blob/2ea3f499b95cb4642a90d619b9a77aa558a577cb/.travis.yml#L23

What would you use Yamllint for? I didn't find any .yml files here.

I'm for support of such CI tools... do you have link to intro post or demo?

soullivaneuh commented 6 years ago

What would you use Yamllint for? I didn't find any .yml files here.

Well, the configuration files. This is why this linter is not mandatory. :-)

I'm for support of such CI tools... do you have link to intro post or demo?

Here is the documentation: https://flintci.io/docs

I can also prepare a PR for you with the needed configuration, all you will have to do is to enable the repository on FlintCI. You just have to be administrator of this one.

The app is still under development, don't hesitate to report any issue with it or the documentation! :+1: https://gitlab.com/flintci/flintci

TomasVotruba commented 6 years ago

I don't see added value here, yet. We've got PHP covered.

soullivaneuh commented 6 years ago

The added values I see right now are:

The possible later added value would be to reduce Travis resource and time usage by using only FlintCI for linting and fixing. But this can be done when all the tools you use will be implemented by the app.

What do you think?

TomasVotruba commented 6 years ago

I see. Imagine scenario:

Travis and composer fix-cs handle this in expected way.

Also I don't think spamming PRs on every commit is good idea. That why I didn't use coveralls for long time, until there's was added option to disable it.

To you tool: what exact problem do you try to solve?

soullivaneuh commented 6 years ago

Well, it depends the way you work with your repo (I usually use forks nor register on apps), but I understand your concern, this is why this is optional.

To you tool: what exact problem do you try to solve?

Quite simple: Provide an app to automatically lint and fix your code using multiple tools as possible. Some already exists, but they required extra configs (a .yml files instead of .php_cs for example) or works only for PR.

As you want for the integration, just close the issue if you don't want to try it now. :wink:

TomasVotruba commented 6 years ago

I don't want to judge something juste because I don't understand it yet. I've done it many times and it doesn't work :)

If I understand it correclty, the configuration for static analysis setup would not be in this repository and 3 configs and not checked by Travis CI, but in 1 config and in another CI service?

I'm asking because adding extra CI service is risky as adding a dependency - people have to learn it, it has it's use cases when it fails and the *benefit of such service must be more than maintenance cost it adds.

I've tried similar CI services and I've always returned to Travis CI that is most spread and well-known CI service here. Easy to maintain and easy to extend.

TomasVotruba commented 6 years ago

I think your makes sense to add when the repository:

But if it automatically fix PHPStan reports (like Psalm does), that would be great thing to add here!

soullivaneuh commented 6 years ago

not checked by Travis CI,

Indeed, it will be the role of FlintCI.

but in 1 config and in another CI service?

Not really. You will have 1 config to tell which service you want to run (e.g. php-cs-fixer), and then the config of the concerned tools (e.g. .php_cs.dist for php-cs-fixer).

Like this, you are not dependent to FlintCI. If you want to remove it later, you can just remove .flintci.yml and keep the other tools file without any break. :+1:

people have to learn it

Yes and no. Because FlintCI is only running the choosed tool, people can still use the "classic way" by running php-cs-fixer for example.

On the reverse, if people is not familiar with php-cs-fixer, he can just download the proposed diff file of FlintCI to apply it.

the author doesn't know much about these tools, nor has written one ;)

Well, indeed! :laughing: Maybe your tool could be added to FlintCI though? :stuck_out_tongue:

But if it automatically fix PHPStan reports (like Psalm does)

Didn't know about this tool, I added it to the todo-list: https://gitlab.com/flintci/flintci/issues/111 (BTW, you may ask for other tools if you want).

But it looks like more a linter than a fixer, isn't it?

soullivaneuh commented 6 years ago

BTW, I would like to write a binary that will fetch and run the configured tools on cli to even simplify the process. But it's still a project for now. :wink:

TomasVotruba commented 6 years ago

Well, since we don't use phpcs directly here, it would not help.

Still feels like duplication of already working setup here.

But it looks like more a linter than a fixer, isn't it?

Yep, fixer might be added value of your tool I'd use (not only) here :+1: If that's ready, feel free to open PR with that here.

I'm closing this now for reasons I've mentioned above.