Skip to content
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

Coding style fixes #7803

Closed
wants to merge 4 commits into from

Conversation

@TomasVotruba
Copy link
Contributor

TomasVotruba commented Nov 24, 2018

This is just basic code cleanup:

  • remove unused imports
  • sort imports a-z
  • remove unused variables
  • remove unused ()

Let me know if you'd like to put there into coding standard setup, so they can be check by CI.

@localheinz

This comment has been minimized.

Copy link
Contributor

localheinz commented Nov 24, 2018

@TomasVotruba

Maybe enable a few more fixers in .php_cs?

@TomasVotruba

This comment has been minimized.

Copy link
Contributor Author

TomasVotruba commented Nov 24, 2018

That's not possible ATM. Many of them are sniffs or 3rd party fixers: https://github.com/Symplify/Symplify/blob/master/packages/EasyCodingStandard/config/clean-code.yml

AFAIK they require PHP 7.1

@TomasVotruba TomasVotruba force-pushed the TomasVotruba:cs-clean branch from 6825984 to 8739591 Nov 24, 2018
@TomasVotruba

This comment has been minimized.

Copy link
Contributor Author

TomasVotruba commented Nov 24, 2018

I'll update the ruleset with what's possible.

I can't find the command that checks coding stadnard. I looked to .travis.yml and composer.json. Where is it?

@TomasVotruba TomasVotruba force-pushed the TomasVotruba:cs-clean branch from 8739591 to 1f1d542 Nov 24, 2018
@localheinz

This comment has been minimized.

Copy link
Contributor

localheinz commented Nov 24, 2018

@TomasVotruba

I think it’s not integrated yet, and only run occasionally. I’m 👍 for an integration into CI.

@TomasVotruba

This comment has been minimized.

Copy link
Contributor Author

TomasVotruba commented Nov 24, 2018

@localheinz It's like having tests without phpunit run in Travis CI :D

@Seldaek

This comment has been minimized.

Copy link
Member

Seldaek commented Nov 26, 2018

Thanks but no. I don't want this automated as it's annoying to contributors and tends to create conflicts unnecessarily. I run CS fixes myself when appropriate. This is the job of the maintainer.

@Seldaek Seldaek closed this Nov 26, 2018
@TomasVotruba

This comment has been minimized.

Copy link
Contributor Author

TomasVotruba commented Nov 26, 2018

Why closing the ruleset update then?

@Seldaek

This comment has been minimized.

Copy link
Member

Seldaek commented Nov 26, 2018

Because I have no interest in these changes.

I think this CS fixing trend is going too far tbh.. normalizing every little tiny details means many lines get changed just due to intricate CS fixes, which degrades git blame ability to function, risks conflicts, and overall creates noise and extra work for maintainers. I wish people would send less of these PRs.

@Seldaek

This comment has been minimized.

Copy link
Member

Seldaek commented Nov 26, 2018

Also, I have had to explain this 15 times every time some new person gets excited about fixing CS in every project out there, and they always wonder why because from their point of view it's the right thing to do. So please don't take it too personally, but it is exhausting from my perspective.

@ntzm

This comment has been minimized.

Copy link

ntzm commented Nov 26, 2018

I think it's worth mentioning this in a contributing document then

@Seldaek

This comment has been minimized.

Copy link
Member

Seldaek commented Nov 26, 2018

Yes I was thinking of adding this to CONTRIBUTING.md as that would hopefully save some people from wasting their time on this.

@shalvah

This comment has been minimized.

Copy link
Contributor

shalvah commented Nov 26, 2018

@Seldaek what do you think of a bot that would automatically make this explanation to any new coding style PRs and then close them? I can do that.

@TomasVotruba

This comment has been minimized.

Copy link
Contributor Author

TomasVotruba commented Nov 26, 2018

What's so too far on ordered_imports? It's easier to read the code, when there is some order. It also helps static analysis, thus prevent bugs. I think it's very healthy trend, keeping programmer safe with helful tools like PHPStan.

Also, I have had to explain this 15 times every time some new person gets excited about fixing CS in every project out there, and they always wonder why because from their point of view it's the right thing to do. So please don't take it too personally, but it is exhausting from my perspective.

No troubles. It's just confusing for me to see .php_cs in this project with kind of contradictory attitude. If there was none, I'd not have to waste our both times.

Yes I was thinking of adding this to CONTRIBUTING.md as that would hopefully save some people from wasting their time on this.

I don't expect people to look to CONTIRBUTING.md to find out if coding standard is used only partially.

@TomasVotruba TomasVotruba deleted the TomasVotruba:cs-clean branch Nov 26, 2018
@TomasVotruba

This comment has been minimized.

Copy link
Contributor Author

TomasVotruba commented Nov 26, 2018

I think you missunderstood me, I don't ask about adding these code style changes.
I asked about the php_cs ruleset.

I don't mind dropping the changes and I understand that 👍

I'm sorry if it's sensitive topic for you, I didn't mean no harm.

@Seldaek

This comment has been minimized.

Copy link
Member

Seldaek commented Nov 26, 2018

I understood you, butI don't feel like debating in depth the value of every single one of PHP CS Fixer's million options. I feel like what we have is enough, if you disagree I am sorry but that's how it is.

@shalvah thanks but I don't think that will be necessary, we don't get that many of them.

Anyway locking this now and going back to work..

@composer composer locked as resolved and limited conversation to collaborators Nov 26, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
5 participants
You can’t perform that action at this time.