Skip to content

Normalize PHP CS to enforce PSR-2 standard and normalized indentation, part 1#6847

Closed
mvorisek wants to merge 2 commits intophp:masterfrom
mvorisek:fix_basic_psr2
Closed

Normalize PHP CS to enforce PSR-2 standard and normalized indentation, part 1#6847
mvorisek wants to merge 2 commits intophp:masterfrom
mvorisek:fix_basic_psr2

Conversation

@mvorisek
Copy link
Copy Markdown
Contributor

@mvorisek mvorisek commented Apr 8, 2021

Part 1, apply on ext/bcmath directory and scripts.

Another parts will update more ext/* files.

Please advise which directories contains files intended to have obscure formating to test PHP lexer.

@KalleZ
Copy link
Copy Markdown
Member

KalleZ commented Apr 9, 2021

I'm not a fan of this, we have been using the PEAR coding standards for php-src and other related PHP.net umbrella projects, on top of this it is an opinionated standard developed outside the PHP project which we have no control over

Copy link
Copy Markdown
Member

@nikic nikic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hard reject for any CI enforced coding style from my side.

@mvorisek
Copy link
Copy Markdown
Contributor Author

mvorisek commented Apr 9, 2021

Hard reject for any CI enforced coding style from my side.

Can you please elaborate what are the cons you see?

@brzuchal
Copy link
Copy Markdown
Contributor

brzuchal commented Apr 9, 2021

IMHO this brings no value and there is no reason to purify code in tests.
The only place I'd see it bringing benefit would be to standardize documentation examples as only these are visible outside

@mvorisek
Copy link
Copy Markdown
Contributor Author

mvorisek commented Apr 9, 2021

Citing from PSR-12 spec why enforcing CS:

Like PSR-2, the intent of this specification is to reduce cognitive friction when scanning code from different authors. It does so by enumerating a shared set of rules and expectations about how to format PHP code. This PSR seeks to provide a set way that coding style tools can implement, projects can declare adherence to and developers can easily relate to between different projects. When various authors collaborate across multiple projects, it helps to have one set of guidelines to be used among all those projects. Thus, the benefit of this guide is not in the rules themselves but the sharing of those rules.

source: https://www.php-fig.org/psr/psr-12/

@cmb69
Copy link
Copy Markdown
Member

cmb69 commented Apr 9, 2021

These changes would do more harm than good, due to inevitable merge conflicts.

@mvorisek
Copy link
Copy Markdown
Contributor Author

mvorisek commented Apr 9, 2021

These changes would do more harm than good, due to inevitable merge conflicts.

As the tests are rarely changed in previous release branches, I think this is not an issue for php-src :)

In long term, this is reduce the conflicts.

@krakjoe
Copy link
Copy Markdown
Member

krakjoe commented Apr 9, 2021

This is just entirely unnecessary. As already mentioned, it provides no meaningful value.

The code in this repository doesn't constitute a library, or a project, or code you have to interoperate with: None of the things that call us to have coding standards in our PHP projects apply to the code here.

Forcing the application of a standard just makes it less likely that people will bother to contribute.

@krakjoe krakjoe closed this Apr 9, 2021
@mvorisek mvorisek deleted the fix_basic_psr2 branch April 9, 2021 12:14
@kamil-tekiela
Copy link
Copy Markdown
Member

There are things I would fix on one by one basis, but I wouldn't want to enforce a standard in PHP tests. For example, I am annoyed by parentheses around require/include and I remove them whenever I am modifying a test case, or unnecessary indentation of PHP code. This makes the code look better and also contributes to proper code highlighting in my IDE, but enforcing this across all test cases is not wise.

@mvorisek
Copy link
Copy Markdown
Contributor Author

mvorisek commented Apr 9, 2021

@kamil-tekiela my PR is to fix whitespaces first and some basic things like else if to elseif.

Can you elaborate what you mean by one by one basis vs. not enforcing the rules globally? Once we fix something, we want that to be enforced, right?

@kamil-tekiela
Copy link
Copy Markdown
Member

All I meant is that I wouldn't do it for all the files just to enforce a standard. If you find yourself changing an existing test file for some other reason, then take some time to fix the formatting to make it easier to read.

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.

7 participants