chore: Normalize implicit backslahes in single quoted strings internally #7786
Conversation
mvorisek
force-pushed
the
phpcsfixer_fix_implicit_backslashes_consistently
branch
from
0715cc7
to
2bae9c7
Compare
|
Can someone please review and merge this PR? 🙏 |
mvorisek
changed the title
PhpCsFixer ruleset@PhpCsFixer ruleset - normalize implicit backslahes in single quoted strings
|
@keradus can we get your approval here, please? |
mvorisek
force-pushed
the
phpcsfixer_fix_implicit_backslashes_consistently
branch
from
2bae9c7
to
4358acd
Compare
|
@mvorisek no, we have an agreement with @keradus that decisions like this require his review. I said it explicitly before and I don't understand why you ping me 🤷♂️. Well, I kind of understand, I also don't like when my changes wait long, but look at my open PRs - the oldest one will be 1 year old in a few weeks time 🤷♂️. It does not work like this, everyone works at their own pace - nobody pings you for changes in the PRs when changes were requested, we just wait. @keradus currently is not available and you just need to wait patiently, pinging won't help you, trust me 😉. |
mvorisek
force-pushed
the
phpcsfixer_fix_implicit_backslashes_consistently
branch
from
4358acd
to
b1a6859
Compare
|
Hello maintainers, can we get your approval on this PR? Thank you in advance! |
src/RuleSet/Sets/PhpCsFixerSet.php
Outdated
| @@ -119,7 +119,7 @@ public function getRules(): array | |||
| 'single_line_comment_style' => true, | |||
| 'single_line_empty_body' => true, | |||
| 'single_line_throw' => false, | |||
| 'string_implicit_backslashes' => ['single_quoted' => 'ignore'], | |||
There was a problem hiding this comment.
I see change of config as something unexpected, I would need to ask wider question in first place. That is not directly scope of this PR, but needed for me to align how we want to move on here.
Why?
Because for applying this config for our PR , I do not see big value.
But this PR is not only changing config for our repo, but is changing best recommendation we are giving via our exposed ruleset. And for that, I want to agree on those recommendations.
why string_implicit_backslashes is having so many options? why for each syntax we allow to escape/ignore/unescape?
Why not just always escape for double_quoted and heredoc, always unescape for single_quoted (and maybe, maybe, allow to set them to ignore) ?
Out of the blue, I would rather deprecate all options of the rule, looking at it.
There was a problem hiding this comment.
Configs options are for users that want different syntax or possibly needs to align the syntax with another CS fixing tools.
In this PR, let's focus solely in normalizing single quotes. All other string notations are fixed already.
There was a problem hiding this comment.
I want to normalize only for direction that we consider the best practice.
do we have the best practice aligned? is it the current defaults of those options?
There was a problem hiding this comment.
Yes - this PR is to unquote possibly quoted single string backslashes and this is strongly the used style in this project and IMO other projects too.
mvorisek
force-pushed
the
phpcsfixer_fix_implicit_backslashes_consistently
branch
from
b1a6859
to
f74562d
Compare
|
@Wirone do you consider this PR approved by the discussion above? |
|
Guys, can this PR be approved, please? |
mvorisek
force-pushed
the
phpcsfixer_fix_implicit_backslashes_consistently
branch
2 times, most recently
from
2d35bf7
to
c1b78a6
Compare
There was a problem hiding this comment.
about #7786 (comment)
related to #7786 (comment)
I tend to not give approval when I'm on -0.5 side of it, yet don't want to explicitly break it because maybe someone else want to give approval.
For me, ideal flow should be following:
- PR 1: change config only in
.php-cs-fixer.dist.php, can be merged with anyone to approve. I am on -0 for it, but not blocking it. - PR 2: change config in RuleSet. We should align on each option of the rule, align what we consider a best practice for the PHP ecosystem (maybe mimic PER-CS or Symfony, maybe come to it by conclusion of Fixer's core team members
- I do not want to merge this PR as-is, as you suggest to change ruleset we expose to end users, and before doing that I want to have alignment by core members what is the best config. This PR is not having this alignment nowadays (and I suggest to externalise it to not wait for it)
- PR 3: create TODOs to adjust defaults of rule itself
mvorisek
changed the title
@PhpCsFixer ruleset - normalize implicit backslahes in single quoted strings
mvorisek
force-pushed
the
phpcsfixer_fix_implicit_backslashes_consistently
branch
2 times, most recently
from
b0dea49
to
aa4389d
Compare
Wirone
force-pushed
the
phpcsfixer_fix_implicit_backslashes_consistently
branch
from
aa4389d
to
fc760e0
Compare
|
Thanks @mvorisek 🍻! |
related with #7669 fixer introduce