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

DX: new config filename #5607

Merged
merged 1 commit into from Apr 13, 2021
Merged

Conversation

keradus
Copy link
Member

@keradus keradus commented Apr 7, 2021

old files are still allowed. will be disallowed with upgrade msg in #5609

@keradus keradus added this to the 2.19.0 milestone Apr 7, 2021
@mvorisek
Copy link
Contributor

mvorisek commented Apr 7, 2021

I propose .php_cs.php and .php_cs.local.php

@keradus
Copy link
Member Author

keradus commented Apr 7, 2021

dist is common pattern for other tools, let's not introducing something new
php_cs was constantly confused with CodeSniffer, let's get rid of that once and for all

@coveralls
Copy link

coveralls commented Apr 7, 2021

Coverage Status

Coverage decreased (-0.007%) to 91.886% when pulling c704d13 on keradus:new_config_format into 3dff24b on FriendsOfPHP:master.

@mvorisek
Copy link
Contributor

mvorisek commented Apr 7, 2021

But .local. can be much easily ignored in .gitignore, .php_cs (or new equivalent) has priority over dist, but this name is very often not ignored thus not helpful. Let's think big and set the new standard 🚀 🍾

Example of universal & easily understandable .gitignore with good naming:

local
*.local
*.local.*
cache
*.cache
*.cache.*

@julienfalque
Copy link
Member

Do we actually need to support having a "dist" file though? I never saw a valid use case and still fail to imagine one.

@mvorisek
Copy link
Contributor

mvorisek commented Apr 8, 2021

Do we actually need to support having a "dist" file though? I never saw a valid use case and still fail to imagine one.

That is the case. We should be simple filename (without dist) and optionally support one prioritized name for local override.

See my suggestion: https://github.com/FriendsOfPHP/PHP-CS-Fixer/pull/5609/files#r609648455

@julienfalque
Copy link
Member

That is the case.

Please elaborate :)

@mvorisek
Copy link
Contributor

mvorisek commented Apr 8, 2021

That is the case.

Please elaborate :)

see above, dist was used to support local override, but naming is not optimal and thus requires this explanation :) Therefore I think https://github.com/FriendsOfPHP/PHP-CS-Fixer/pull/5609/files#r609648455 is better and allow also more generic exclude, see #5607 (comment)

@julienfalque
Copy link
Member

I know it allows local override, but why is that needed? The point of this tool is to enforce consistent code style between multiple code contributors. When would one need to diverge from that locally (i.e. apply a different code style)?

@mvorisek
Copy link
Contributor

mvorisek commented Apr 8, 2021

I know it allows local override, but why is that needed? The point of this tool is to enforce consistent code style between multiple code contributors. When would one need to diverge from that locally (i.e. apply a different code style)?

I belive to support one DEFAULT local/prioritized name, ie. one name that does not have to be specified explicitly to override the config.

@keradus
Copy link
Member Author

keradus commented Apr 8, 2021

@julienfalque , i know you are not in favour of multiple config files approach ;)

what is dist for: https://stackoverflow.com/a/44764078
don't like dist, don't use it ;) symfony/recipes#41 (comment)

@KacerCZ
Copy link

KacerCZ commented Apr 9, 2021

@mvorisek Use of dist (short from 'distribute') configurations is common in PHP tools. I don't think it is good idea to change this pattern unless you want to change it in all other tools also.

@julienfalque My typical use case for local and dist config is introduction of new rules to project.
Repository contains only dist configuration and CI uses it to check code.
My local configuration is copy of dist with one or more rules enabled.
So when I modify code it allows me to fix more errors.
I don't have time to fix all errors at once (specially risky rules).

@mvorisek
Copy link
Contributor

mvorisek commented Apr 9, 2021

want to change it in all other tools also

I would be happy to do so witch CS Fixer taking the lead :)

@keradus keradus force-pushed the new_config_format branch 3 times, most recently from af8b677 to 25d4306 Compare April 12, 2021 19:45
@keradus keradus merged commit d627acb into PHP-CS-Fixer:master Apr 13, 2021
@keradus keradus deleted the new_config_format branch April 13, 2021 13:15
keradus added a commit that referenced this pull request Apr 13, 2021
This PR was squashed before being merged into the 3.0 branch.

Discussion
----------

Forbid old config filename usage

Depends on:
- [x] #5607
- [x] #5608

Commits
-------

7ba957d Forbid old config filename usage
KacerCZ added a commit to KacerCZ/netbeans that referenced this pull request Apr 13, 2021
jeffwidman added a commit to jeffwidman/dependabot-core that referenced this pull request Sep 12, 2022
Fix deprecation warning by renaming the config file:
```
  15.95 > php-cs-fixer fix --diff --verbose '--dry-run'
  16.10 You are running PHP CS Fixer v2, which is not maintained anymore. Please update to v3.
  16.10 You may find an UPGRADE guide at https://github.com/FriendsOfPHP/PHP-CS-Fixer/blob/v3.3.0/UPGRADE-v3.md .
  16.10 If you need help while solving warnings, ask at https://gitter.im/PHP-CS-Fixer, we will help you!
  16.10
  16.10 PHP CS Fixer 2.19.3 Testament by Fabien Potencier and Dariusz Ruminski
  16.10 Runtime: PHP 7.4.30
  16.10 Loaded config default from "/opt/composer/v1/.php_cs".
  16.14 ......
  16.26 Legend: ?-unknown, I-invalid file syntax (file ignored), S-skipped (cached or empty file), .-no changes, F-fixed, E-error
  16.26
  16.26 Checked all files in 0.134 seconds, 14.000 MB memory used
  16.26
  16.26 Detected deprecations in use:
  16.26 - Configuration file `.php_cs` is deprecated, rename to `.php-cs-fixer.php`.
```

From https://github.com/dependabot/dependabot-core/actions/runs/3040792095/jobs/4897247001#step:6:6736

The warning suggests to rename to `php-cs-fixer.php`. However, looking
at https://github.com/keradus/PHP-CS-Fixer/blob/master/UPGRADE-v3.md I
see that is the recommended file name for the _local development_ config file. The
distributed one should be `.php-cs-fixer.dist.php`, so I used that
instead. More details on the distinction between the two types of files
here: PHP-CS-Fixer/PHP-CS-Fixer#5607
jeffwidman added a commit to jeffwidman/dependabot-core that referenced this pull request Sep 13, 2022
Fix deprecation warning by renaming the config file:
```
  15.95 > php-cs-fixer fix --diff --verbose '--dry-run'
  16.10 You are running PHP CS Fixer v2, which is not maintained anymore. Please update to v3.
  16.10 You may find an UPGRADE guide at https://github.com/FriendsOfPHP/PHP-CS-Fixer/blob/v3.3.0/UPGRADE-v3.md .
  16.10 If you need help while solving warnings, ask at https://gitter.im/PHP-CS-Fixer, we will help you!
  16.10
  16.10 PHP CS Fixer 2.19.3 Testament by Fabien Potencier and Dariusz Ruminski
  16.10 Runtime: PHP 7.4.30
  16.10 Loaded config default from "/opt/composer/v1/.php_cs".
  16.14 ......
  16.26 Legend: ?-unknown, I-invalid file syntax (file ignored), S-skipped (cached or empty file), .-no changes, F-fixed, E-error
  16.26
  16.26 Checked all files in 0.134 seconds, 14.000 MB memory used
  16.26
  16.26 Detected deprecations in use:
  16.26 - Configuration file `.php_cs` is deprecated, rename to `.php-cs-fixer.php`.
```

From https://github.com/dependabot/dependabot-core/actions/runs/3040792095/jobs/4897247001#step:6:6736

The warning suggests to rename to `php-cs-fixer.php`. However, looking
at https://github.com/keradus/PHP-CS-Fixer/blob/master/UPGRADE-v3.md I
see that is the recommended file name for the _local development_ config file. The
distributed one should be `.php-cs-fixer.dist.php`, so I used that
instead. More details on the distinction between the two types of files
here: PHP-CS-Fixer/PHP-CS-Fixer#5607
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.

None yet

7 participants