feature: introduce single_space_around_construct, deprecate single_space_after_construct by keradus · Pull Request #6857 · PHP-CS-Fixer/PHP-CS-Fixer

keradus · 2023-03-24T23:20:49Z

I aimed to solve first TODO item from #4885
I limited to needed changes, I was not redoing the logic of old fixer.

…ion was added later and we didn't want to break BC by changing default option value

coveralls · 2023-03-24T23:42:26Z

Pull Request Test Coverage Report for Build 4516272913

183 of 188 (97.34%) changed or added relevant lines in 3 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.008%) to 93.002%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/Fixer/LanguageConstruct/SingleSpaceAfterConstructFixer.php	11	12	91.67%
src/Fixer/LanguageConstruct/SingleSpaceAroundConstructFixer.php	171	175	97.71%

Totals
Change from base Build 4515154257:	0.008%
Covered Lines:	22711
Relevant Lines:	24420

💛 - Coveralls

keradus · 2023-03-25T00:01:40Z

src/Fixer/LanguageConstruct/SingleSpaceAroundConstructFixer.php

+ * @author Andreas Möller <am@localheinz.com>
+ * @author Dariusz Rumiński <dariusz.ruminski@gmail.com>
+ */
+final class SingleSpaceAroundConstructFixer extends AbstractFixer implements ConfigurableFixerInterface


maybe we should call it SpacingAroundConstructFixer, so in future we would be open for no whitespace before/after given construct, or multiple ones? (eg multi-line instead of single space, for specific cases) ?

Good idea, but probably properties' names should be aligned too ($****SingleSpace). Also FixerDefinition must be changed to not explicitly state "single space" as the only way. In general, a lot to change 😅.

Wirone

Few thoughts, it's hard to review this PR on mobile, so I don't know if I missed something or not 😅.

Wirone · 2023-03-25T13:07:50Z

src/Fixer/LanguageConstruct/SingleSpaceAfterConstructFixer.php

+
    /**
-     * @var array<string, int>
+     * {@inheritdoc}


Side note, nothing to be done: I find such phpDocs with only {@inheritdoc} superfluous.

to be considered as practice to use or not to use them for whole project. probably a fixer, if we want to remove them.

Wirone · 2023-03-25T13:13:56Z

src/Fixer/LanguageConstruct/SingleSpaceAroundConstructFixer.php

+     */
+    private static array $tokenMapContainASingleSpace = [
+        // for now, only one case - but we are ready to extend it, when we learn about new cases to cover
+        'yield_from' => T_YIELD_FROM,


Shouldn't const_import and function_import be here?

no. they do not contain whitespace.

They do? Is it for use function, use const? 🤔

yield from is ONE token, will be fixed by contain option

use function are THREE tokens, will be fixed by preceded / followed

ref https://3v4l.org/mW39A

Wirone · 2023-03-25T13:17:05Z

src/Fixer/LanguageConstruct/SingleSpaceAroundConstructFixer.php

+ * @author Andreas Möller <am@localheinz.com>
+ * @author Dariusz Rumiński <dariusz.ruminski@gmail.com>
+ */
+final class SingleSpaceAroundConstructFixer extends AbstractFixer implements ConfigurableFixerInterface


Good idea, but probably properties' names should be aligned too ($****SingleSpace). Also FixerDefinition must be changed to not explicitly state "single space" as the only way. In general, a lot to change 😅.

Wirone · 2023-03-25T13:20:35Z

src/Fixer/LanguageConstruct/SingleSpaceAroundConstructFixer.php

+            }
+        }
+
+        $this->fixTokenMapPrecededByASingleSpace = [];


Property has [] as default value, I believe it's not needed to assign it here and we can just iterate over configuration array?

Same for other properties below.

If it's just for safety (calling configure() multiple times), then 👍.

yes. it's part of public interface and one can call it any moment. we do it like this for any rule.

Wirone · 2023-03-25T13:30:10Z

src/Fixer/LanguageConstruct/SingleSpaceAroundConstructFixer.php

+
+            $whitespaceTokenIndex = $index + 1;
+
+            if ($tokens[$whitespaceTokenIndex]->equalsAny([',', ';', ')', [CT::T_ARRAY_SQUARE_BRACE_CLOSE], [CT::T_DESTRUCTURING_SQUARE_BRACE_CLOSE]])) {


Can we extract all these conditions that do continue to separate method and call it here for better readability?

copy-pasted from SingleSpaceAfterConstructFixer. goal of this PR is not refactoring of existing logic, but to make final step to deprecate BracesFixer at lowest remaining effort, as described in opening msg. If you up for refactoring, please raise a (sub?) PR

I limited to needed changes, I was not redoing the logic of old fixer.

Wirone · 2023-03-25T13:33:11Z

src/Fixer/LanguageConstruct/SingleSpaceAroundConstructFixer.php

+        return false;
+    }
+
+    private function isMultilineExtendsOrImplementsWithMoreThanOneAncestor(Tokens $tokens, int $index): bool


This looks like something that may be helpful in other fixers, so maybe we can extract it somewhere? Same with isMultilineConstant().

same as #6857 (comment)

Wirone · 2023-03-25T13:34:52Z

tests/AutoReview/FixerFactoryTest.php

@@ -491,6 +491,7 @@ private static function getFixersPriorityGraph(): array
                'not_operator_with_successor_space',
                'php_unit_dedicate_assert',
                'single_space_after_construct',


Isn't it a part of single_space_around_construct?

as long as both fixers exist, both should be tested. including the priority of them.

extra points for our self-tests to detect if it's missing and fail another test in such case.

Wirone · 2023-03-25T13:38:26Z

src/Fixer/LanguageConstruct/SingleSpaceAroundConstructFixer.php

+    /**
+     * @var array<string, null|int>
+     */
+    private static array $tokenMapContainASingleSpace = [


These properties are used statically only in configure() which is not static, what is the advantage of having them defined like that?

another $tokenMap that was already existing is declared static in master branch, moved it as-is and kept the pattern.
I guess the initial reason was that they are same for any instance of this class.

keradus added 6 commits March 25, 2023 00:18

copy fixer

33dcf93

fix type_colon by default. it was not by default earlier, as this opt…

efd2a51

…ion was added later and we didn't want to break BC by changing default option value

introduce constructs_preceded_by_a_single_space

3ca5908

externalize constructs_contain_a_single_space option

425d08a

deprecate SingleSpaceAfterConstructFixer

6136d60

regenerate docs

fb91b6a

keradus marked this pull request as ready for review March 24, 2023 23:20

keradus mentioned this pull request Mar 24, 2023

minor: Deprecate BracesFixer #4885

Merged

2 tasks

keradus added 2 commits March 25, 2023 00:49

please phpstan

55e8b83

restore phpunit config

be8902f

keradus commented Mar 25, 2023

View reviewed changes

Wirone reviewed Mar 25, 2023

View reviewed changes

keradus merged commit e04eef3 into PHP-CS-Fixer:master Mar 29, 2023

keradus deleted the single_space_around_construct branch March 29, 2023 22:12


		$whitespaceTokenIndex = $index + 1;

		if ($tokens[$whitespaceTokenIndex]->equalsAny([',', ';', ')', [CT::T_ARRAY_SQUARE_BRACE_CLOSE], [CT::T_DESTRUCTURING_SQUARE_BRACE_CLOSE]])) {

Uh oh!

Conversation

keradus commented Mar 24, 2023

Uh oh!

coveralls commented Mar 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 4516272913

💛 - Coveralls

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Wirone left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keradus Mar 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keradus Mar 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

coveralls commented Mar 24, 2023 •

edited

Loading

keradus Mar 25, 2023 •

edited

Loading

keradus Mar 25, 2023 •

edited

Loading