Skip to content

CS: reapply fixers #12192

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

CS: reapply fixers #12192

wants to merge 1 commit into from

Conversation

keradus
Copy link
Contributor

@keradus keradus commented Nov 11, 2017

Hi !
I wanted to do sth with repo, and then executed boundled PHP CS Fixer config. Yet, it crashes on a lot of violation. So let me extract the CS changes to initial PR from my side containing only them.

Probably it would be good to add PHP CS Fixer integration into Travis later (I could help with it if it would be wanted).

This PR is only automated result of "php-cs-fixer fix .` using config file

@keradus
Copy link
Contributor Author

keradus commented Nov 11, 2017

static job is failing as it's too slow for Travis this time

@orlangur orlangur self-assigned this Nov 13, 2017
@orlangur
Copy link
Contributor

Hi @keradus, glad to see you again!

Back to the #10770 (comment), are you going to contribute a mechanism enforcing these rules? Without such mechanism applying coding style fixes does not make a lot of sense. I don't think wrapping it as PHPUnit-based test is the only option, so, if you see a better approach which can work fine with Travis+Bamboo+Jenkins, please share some thoughts.

@keradus
Copy link
Contributor Author

keradus commented Nov 13, 2017

Hi ;) Thank you for warm welcome!

I would suggest to integrate it with one (no need for multiple) CI service that is there. I could send a PR to implement it into Travis. Totally outside of phpunit.
So, if one would run it locally, first run is slow, but it builds cache, next run is fast. On Travis, each run would touch only modified files.
If interoperability between different CI is must have, we could wrap logic of gathering files to check and running the command into few-liner shell script.
If you wish, I could prepare a dedicated PR.

But for that, let us first fix current violations.

@orlangur
Copy link
Contributor

Unassigning from myself for now, I'm quite interested in such cleanup and enforcing it with test (probably list of rules should be revised before apply).

Just that I need to round out a bunch of other PRs. Will raise a maintainers discussion on appropriate implementation approach after this if nobody do it before.

@orlangur orlangur removed their assignment Nov 20, 2017
@keradus
Copy link
Contributor Author

keradus commented Nov 20, 2017

OK, thanks for info.
So now, please guide me, is there anything for now I could help with?

@orlangur
Copy link
Contributor

I think all is good from your side, approach upon merging such changes should be agreed first, I did an unsuccessful attempt of reapply in scope of #9367 (comment) earlier.

@ishakhsuvarov
Copy link
Contributor

@keradus Unfortunately we can not accept such big amount of changes in a single PR as it requires manual review of 4300+ files, which is nearly impossible.
Additionally, this PR requires all queue to be stopped, as merge conflicts will arise nearly for every other PR merged.
I am closing this for now, until we discuss the flow to merge this kind of PR's. We will reopen and let you know if we manage to work out a flow for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants