docs: Sort options in documentation #7345

localheinz · 2023-09-29T14:22:49Z

This pull request

adjusts the FixerConfigurationResolver to keep options sorted by name
regenerates the documentation

src/Documentation/FixerDocumentGenerator.php

SpacePossum · 2023-10-04T08:34:49Z

Code looks clean and good to me.
I do wonder, options sorted like these:

after_argument_assignments
after_array_assignments_colon
after_array_assignments_equals
around_commas
around_parentheses
before_argument_assignments
before_array_assignments_colon
before_array_assignments_equals
ignored_tags

is that more helpful than lets say:

before_argument_assignments
after_argument_assignments
before_array_assignments_colon
after_array_assignments_colon
before_array_assignments_equals
after_array_assignments_equals
around_commas
around_parentheses
ignored_tags

point could be made that maybe the name could've been better in hindsight

argument_assignments_after
argument_assignments_before
[...]

but yeah, don't know, just wanted to share my thoughts on it

keradus · 2023-10-07T19:05:47Z

argument_assignments_after
argument_assignments_before

I like this idea, if it's would also fit on the grammar side (i hope for most cases it would). maybe worth to create a follow up - but it could be bigger effort have all those proxy/deprecated options, for not that big gain.

Without having this in place, I would suggest to keep alpha sorting as simples option

keradus

Like the idea and your own suggestion how to improve further ;)

localheinz · 2023-10-09T21:16:53Z

@keradus @SpacePossum

There are also other options, e.g.

max_line_breaks
min_line_breaks

where special ordering could make sense.

localheinz · 2023-10-09T21:27:09Z

tests/Fixer/ClassNotation/ClassDefinitionFixerTest.php

+            'inline_constructor_arguments' => true,
            'multi_line_extends_each_single_line' => false,
            'single_item_single_line' => false,
            'single_line' => false,
            'space_before_parenthesis' => false,
-            'inline_constructor_arguments' => true,


@keradus @SpacePossum

As an alternative to this change, I could extract a FixerOptionSorter and use it in both the FixerConfigurationResolver and ClassDefinitionFixerTest::assertSame().

What do you think?

I think we either have a low-level test that options are always sorted,
or we have a sorter mechanism that is used on that low-level (ref #7345 (comment) )

Having a test that checks all options for all fixers seems the way to go for me. Than there is no need to test order of the options @ runtime. (likely in FixerConfigurationResolverTest.php)

There are also other options, e.g. max_line_breaks min_line_breaks where special ordering could make sense.

👍 that could be added to the tests, I think I a saw an option called default somewhere as well, maybe it might make sense to have that one as first as well for example. I just thinking out loud here

@keradus @SpacePossum

Not sure I understand - would you like me to to make any changes here? If so, which? Thank you!

Think keradus has other idea than I ;)
I have no strong opinion on the implementation of this, I think the idea is great!
So I'll leave the implementation details to you two :)

coveralls · 2023-10-09T21:38:34Z

coverage: 94.597% (+0.02%) from 94.581% when pulling aa9e14d on localheinz:feature/sort into 0607649 on PHP-CS-Fixer:master.

keradus · 2023-10-10T10:10:10Z

tests/FixerConfiguration/FixerConfigurationResolverTest.php

@@ -65,13 +65,22 @@ public function testWithDuplicateAliasOptions(): void

    public function testGetOptions(): void


keradus

please look into failing CI

localheinz · 2023-10-12T13:14:34Z

Thank you, @keradus and @SpacePossum!

localheinz self-assigned this Sep 29, 2023

localheinz force-pushed the feature/sort branch from 8983c5e to 7fcac76 Compare September 29, 2023 14:24

localheinz commented Sep 29, 2023

View reviewed changes

src/Documentation/FixerDocumentGenerator.php Outdated Show resolved Hide resolved

localheinz force-pushed the feature/sort branch 2 times, most recently from 42bf395 to c58b835 Compare September 29, 2023 14:30

localheinz marked this pull request as ready for review September 29, 2023 14:37

localheinz removed their assignment Sep 29, 2023

keradus changed the title ~~feat: Sort options in documentation~~ docs: Sort options in documentation Oct 7, 2023

keradus approved these changes Oct 7, 2023

View reviewed changes

localheinz self-assigned this Oct 9, 2023

localheinz marked this pull request as draft October 9, 2023 19:35

localheinz force-pushed the feature/sort branch 2 times, most recently from c9e27ee to 62f5847 Compare October 9, 2023 21:09

localheinz marked this pull request as ready for review October 9, 2023 21:14

localheinz commented Oct 9, 2023

View reviewed changes

localheinz removed their assignment Oct 9, 2023

localheinz requested a review from keradus October 9, 2023 21:28

keradus reviewed Oct 10, 2023

View reviewed changes

localheinz force-pushed the feature/sort branch 2 times, most recently from 82abf43 to 9443801 Compare October 11, 2023 09:37

keradus approved these changes Oct 11, 2023

View reviewed changes

localheinz added 4 commits October 12, 2023 10:21

Enhancement: Sort options in documentation

0f04aff

Fix: Regenerate documentation

ee05dfd

Enhancement: Move sorting to FixerConfigurationResolver

e1dedc1

Fix: Regenerate documentation

de0899c

localheinz added 6 commits October 12, 2023 10:21

Fix: Adjust failing tests

d4d12cc

Fix: Slide statement

a9800eb

Fix: Adjust failing test

0015e39

Enhancement: Extract FixerOptionSorter

8781873

Fix: Mark class as internal

2f160aa

Fix: Namespace

aa9e14d

localheinz force-pushed the feature/sort branch from 17c6c6e to aa9e14d Compare October 12, 2023 08:21

keradus merged commit 6d3c3bc into PHP-CS-Fixer:master Oct 12, 2023
15 checks passed

localheinz deleted the feature/sort branch October 12, 2023 13:14

kubawerlos mentioned this pull request Oct 21, 2023

Improve wording in README kubawerlos/php-cs-fixer-custom-fixers#915

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docs: Sort options in documentation #7345

docs: Sort options in documentation #7345

localheinz commented Sep 29, 2023 •

edited

Loading

SpacePossum commented Oct 4, 2023 •

edited

Loading

keradus commented Oct 7, 2023

keradus left a comment

localheinz commented Oct 9, 2023

localheinz Oct 9, 2023

keradus Oct 10, 2023

SpacePossum Oct 10, 2023

localheinz Oct 10, 2023

SpacePossum Oct 10, 2023

coveralls commented Oct 9, 2023 •

edited

Loading

keradus Oct 10, 2023

keradus left a comment

localheinz commented Oct 12, 2023

		@@ -65,13 +65,22 @@ public function testWithDuplicateAliasOptions(): void

		public function testGetOptions(): void

docs: Sort options in documentation #7345

docs: Sort options in documentation #7345

Conversation

localheinz commented Sep 29, 2023 • edited Loading

SpacePossum commented Oct 4, 2023 • edited Loading

keradus commented Oct 7, 2023

keradus left a comment

Choose a reason for hiding this comment

localheinz commented Oct 9, 2023

localheinz Oct 9, 2023

Choose a reason for hiding this comment

keradus Oct 10, 2023

Choose a reason for hiding this comment

SpacePossum Oct 10, 2023

Choose a reason for hiding this comment

localheinz Oct 10, 2023

Choose a reason for hiding this comment

SpacePossum Oct 10, 2023

Choose a reason for hiding this comment

coveralls commented Oct 9, 2023 • edited Loading

keradus Oct 10, 2023

Choose a reason for hiding this comment

keradus left a comment

Choose a reason for hiding this comment

localheinz commented Oct 12, 2023

localheinz commented Sep 29, 2023 •

edited

Loading

SpacePossum commented Oct 4, 2023 •

edited

Loading

coveralls commented Oct 9, 2023 •

edited

Loading