PHP-CS-Fixer icon indicating copy to clipboard operation
PHP-CS-Fixer copied to clipboard

feat: Introduce `OrderedAttributesFixer`

Open HypeMC opened this issue 2 years ago • 3 comments

Sorts attributes:

<?php

+#[Bar(3)]
+#[Corge(a: 'test')]
 #[Foo]
-#[Bar(3)]
 #[Qux(new Bar(5))]
-#[Corge(a: 'test')]
 class Sample1 {}

#[
+    Bar(3),
+    Corge(a: 'test'),
     Foo,
-    Bar(3),
     Qux(new Bar(5)),
-    Corge(a: 'test'),
]
class Sample2 {}

Inspired by https://github.com/rectorphp/rector-src/pull/3838

HypeMC avatar Oct 29 '23 16:10 HypeMC

Coverage Status

coverage: 96.122% (+0.009%) from 96.113% when pulling b9ca5bbca408b62741a7be4b4a62c6909467989b on HypeMC:ordered-attributes-fixer into bb387e163ac5823adbe4c6a630867b0ce264312e on PHP-CS-Fixer:master.

coveralls avatar Oct 29 '23 16:10 coveralls

@HypeMC I've rebased your branch to keep PR up to date and it seems like it requires some attention - we recently introduced auto-review test that enforces tests' naming convention, can you align the code here? Sorry for the delay, I plan to revisit this PR soon because it's a really good feature worth merging 🙂.

Wirone avatar Dec 14 '23 16:12 Wirone

@Wirone No problem, done.

HypeMC avatar Dec 14 '23 20:12 HypeMC

@Wirone I've rebased the PR now that #7909 was merged. I've also extracted the logic to determine attribute FQCNs into a separate class for reusability. If you don't think this is useful, I'll just revert the commit.

HypeMC avatar Apr 06 '24 06:04 HypeMC

@HypeMC I'll remove last commit because FQCN resolver seems to be incomplete, as it does not resolve properly names when multiple namespaces are used:

image

Also, it should be FullyQualifiedNameResolver because:

  • Abbreviations should be treated as words (Fqcn, not FQCN) in order to be able to convert camelCase to snake_case and keep the meaning (with current name it would be class_f_q_c_n_resolver)
  • C in FQCN stands for Class, so ClassFqcnResolver has double "class"
  • it can be fully qualified name of interface too, so I would avoid using class in resolver's name completely

I don't want to block this PR longer, so let's extract such resolver separately - SRP also applies to PRs 🙂.

Wirone avatar Apr 11 '24 21:04 Wirone

@HypeMC I did some refactoring to shorten the feedback cycle. It's close to what I would consider ready for merge, I want to try refactor one more thing but I had some problem and won't finish it now. Anyway, one concern: with this implementation (my refactor does not matter here) namespace analysis will be performed for every attribute (so the same work will be done multiple times within the same namespace), which is not good from performance perspective. However, it's only for none sorting algorithm, so maybe not that critical 😅.

Wirone avatar Apr 11 '24 23:04 Wirone

I've renamed strategy to custom as this reflects the intent better. none strategy is... when you don't use this rule 😉.

Wirone avatar Apr 28 '24 13:04 Wirone

@Wirone No problem, thank you for the reviews :smile:

HypeMC avatar Apr 28 '24 14:04 HypeMC

Wdyt about adding a new syntax for custom order '*' ?

Allowing something like ['sort_algorithm' => 'custom', 'order' => ['A\\B\\Qux', 'A\\B\\Bar', '*', 'A\\B\\Corge']].

Which means "Qux first, then Bar, then everything then Corge" ?

VincentLanglet avatar Apr 30 '24 11:04 VincentLanglet

@VincentLanglet looks interesting. Basically you would like to enforce what's at the top and what's at the bottom, and the rest in the middle in the exact same order as they currently are?

Wirone avatar Apr 30 '24 12:04 Wirone

@VincentLanglet looks interesting. Basically you would like to enforce what's at the top and what's at the bottom, and the rest in the middle in the exact same order as they currently are?

Yes, in the same way, enforcing just the bottom or the top would be possible with

['sort_algorithm' => 'custom', 'order' => ['*', 'A\\B\\Corge']]

or

['sort_algorithm' => 'custom', 'order' => ['A\\B\\Qux', 'A\\B\\Bar', '*']]

I dunno if people will come up also asking for something like

['sort_algorithm' => 'custom', 'order' => ['A\\B\\*', '*']]

ie: all the attributes in the namespace A\B then the others. This would avoid having to list every attributes.

VincentLanglet avatar May 01 '24 08:05 VincentLanglet

Do you want a feature request @Wirone ?

VincentLanglet avatar May 02 '24 09:05 VincentLanglet

@VincentLanglet Yes, please 🙂. You can link your comment in the issue form (instead of discussion).

Wirone avatar May 06 '24 11:05 Wirone

Done https://github.com/PHP-CS-Fixer/PHP-CS-Fixer/issues/7993

VincentLanglet avatar May 08 '24 15:05 VincentLanglet