[TASK] Update our coding standard #1264

oliverklee · 2024-06-06T14:25:17Z

Switch to the PER2 coding standard and reduce the number of custom rules.

This basically aligns our code style with that used by the PHP-CSS-Parser project, which is very close to the PER-2 coding standard.

Fixes #1262

oliverklee · 2024-06-12T07:40:52Z

It looks like I'll need to tweak the ruleset a bit to retain compatibility with PHP 7.3 and 7.4.

oliverklee · 2024-06-22T07:23:44Z

@JakeQZ friendly ping :-)

JakeQZ · 2024-06-22T22:57:03Z

@JakeQZ friendly ping :-)

Sorry, I missed this amongst the other 464 emails in the last 5 days. Not even exaggerating - I just counted them ;)

JakeQZ

I'll need to go through the removed settings in more detail, which will take a bit of time I don't have right now. But there is the match PHP 8 issue.

JakeQZ · 2024-06-22T23:01:14Z

config/php-cs-fixer.php

-            'no_unused_imports' => true,
-            'ordered_imports' => true,
+            // overwrite the PER2 defaults to restore compatibility with PHP 7.x
+            'trailing_comma_in_multiline' => ['elements' => ['arrays', 'match']],


As per issue on sister project, we can't include match here, until PHP-CS-Fixer fixes the issue.

It's removed now.

The match doesn't appear to be removed from where I'm looking.

(BTW, I've realized now that the defaults haven't changed with the new PHP-CS-Fixer version, it's rather than it's included in the @PER-CS2.0 rule set - and that may have changed between Fixer versions.)

Oh, it seems match was there 2 times. I've removed the other one now as well, and also created MyIntervals/PHP-CSS-Parser#629.

JakeQZ

I've been through all the added and removed rules, and checked what the defaults are, and whether they are included in any of the rule sets that will be active following this change.

Quite a few of the removed rules I think we need to keep, because they are not applied by default, nor included in an rule set.

Many of the other removed rules seemed to be incorrectly specified as a Boolean, when in fact what is expected is an array of configuration values. I've ignored those, assuming they were invalid and would have had no effect anyway. I don't know if I'm missing something, like if it's possible to provide a Boolean when an array is expected, and that would result in enabling all options (though in some cases the options aren't Boolean either).

Some of the added rules appear unnecessary as they are the default values anyway. At least one of the added rules is given as a Boolean when an array is expected.

JakeQZ · 2024-06-24T21:49:49Z

config/php-cs-fixer.php

-            // casing
-            'magic_constant_casing' => true,
-            'native_function_casing' => true,


I think we need to keep these, since they are not part of any of the rule sets.

GitHub diff isn't highlighting the lines I selected, so you'll need to check the smallprint, e.g. "Comment on lines -39 to -41" to see how many preceding lines I'm referring to. If there's nothing there, it means I'm just commenting on a single line.

JakeQZ · 2024-06-24T21:53:04Z

config/php-cs-fixer.php

-            'modernize_types_casting' => true,
-            'no_short_bool_cast' => true,


Ditto these two.

JakeQZ · 2024-06-24T21:55:01Z

config/php-cs-fixer.php

-            // class notation
-            'class_attributes_separation' => true,
-            'no_blank_lines_after_class_opening' => true,
-            'no_php4_constructor' => true,


And this one.

JakeQZ · 2024-06-24T21:56:26Z

config/php-cs-fixer.php

-            // (no rules used from this section)
-
-            // comment
-            'no_empty_comment' => true,


And this one.

JakeQZ · 2024-06-24T22:08:58Z

config/php-cs-fixer.php

-
-            // control structure
-            'elseif' => true,
-            'no_superfluous_elseif' => true,


This one...

JakeQZ · 2024-06-24T23:05:19Z

config/php-cs-fixer.php

-            'phpdoc_separation' => true,
-            'phpdoc_trim' => true,
-            'phpdoc_types' => true,
-            'phpdoc_types_order' => ['null_adjustment' => 'always_last', 'sort_algorithm' => 'none'],


JakeQZ · 2024-06-24T23:05:56Z

config/php-cs-fixer.php

-            'phpdoc_types_order' => ['null_adjustment' => 'always_last', 'sort_algorithm' => 'none'],
-
-            // return notation
-            'no_useless_return' => true,


JakeQZ · 2024-06-24T23:07:03Z

config/php-cs-fixer.php

-            'no_empty_statement' => true,
-            'no_singleline_whitespace_before_semicolons' => true,
-            'semicolon_after_instruction' => true,


And these three.

JakeQZ · 2024-06-24T23:07:29Z

config/php-cs-fixer.php

-            'space_after_semicolon' => true,
-
-            // strict
-            'declare_strict_types' => true,


JakeQZ · 2024-06-24T23:11:46Z

config/php-cs-fixer.php

-            'declare_strict_types' => true,
-
-            // string notation
-            'escape_implicit_backslashes' => ['single_quoted' => true],


This should be replaced with string_implicit_backslashes => ['single_quoted' => 'escape'], which isn't in any of the rule sets used (escape_implicit_backslashes is deprecated).

Switch to the PER2 coding standard and reduce the number of custom rules. This basically aligns our code style with that used by the PHP-CSS-Parser project, which is very close to the PER-2 coding standard. Fixes #1262

oliverklee · 2024-06-29T08:07:27Z

@JakeQZ Thank your for doing the painstaking work of going through this with a fine-grained comb! 🙏 I've now updated the ruleset accordingly.

JakeQZ

I think I've checked most if not all the initial review change suggestions. However, it's tricky since GitHub drops some of the original review comments from the diff if it can't work out where to place them now. (In some cases that's quite poor - e..g. lines added then removed - the review comment disappears too.)

One thing I have spotted: if we don't want no_useless_else, we probably also don't want no_superfluous_elseif, for consistency.

JakeQZ · 2024-06-30T00:26:04Z

config/php-cs-fixer.php

-            'no_superfluous_elseif' => true,
-            'no_unneeded_control_parentheses' => true,
-            'no_unneeded_curly_braces' => true,
-            'no_useless_else' => true,


On that basis, we also need to remove the no_superfluous_elseif.

JakeQZ · 2024-06-30T00:27:08Z

config/php-cs-fixer.php


            // control structure
-            'elseif' => true,
            'no_superfluous_elseif' => true,


If we remove no_useless_else, we should also remove no_superfluous_elseif.

JakeQZ

Having finalized this for now (it will change in future, I'm sure) here, we should probably update the equivalent on the sister project to match. And like the contrubution guidelines, CoC, and other bits and bobs, keep them in sync.

I wonder if there's a way of using a template to keep cross-project files like these in sync. You mentioned that the CoC came from a GitHub template (I think). Can we introduce our own repository-owner templates to help?

oliverklee added the cleanup label Jun 6, 2024

oliverklee added this to the 8.0.0 milestone Jun 6, 2024

oliverklee requested a review from JakeQZ June 6, 2024 14:25

oliverklee self-assigned this Jun 6, 2024

oliverklee force-pushed the task/fixer-rules branch from 80e254d to fceabc1 Compare June 12, 2024 07:39

oliverklee marked this pull request as draft June 12, 2024 07:40

oliverklee force-pushed the task/fixer-rules branch 3 times, most recently from d3a69f5 to 7590cb4 Compare June 19, 2024 17:45

oliverklee marked this pull request as ready for review June 19, 2024 17:47

JakeQZ requested changes Jun 22, 2024

View reviewed changes

oliverklee force-pushed the task/fixer-rules branch from f78f3be to 42b7359 Compare June 23, 2024 09:58

JakeQZ requested changes Jun 24, 2024

View reviewed changes

oliverklee added 4 commits June 29, 2024 09:56

[TASK] Update our coding standard

2a5ac78

Switch to the PER2 coding standard and reduce the number of custom rules. This basically aligns our code style with that used by the PHP-CSS-Parser project, which is very close to the PER-2 coding standard. Fixes #1262

Disallow trailing comma after multiline heredoc

facea01

Make the PHP 7 thing more clear

2ca4773

Change ruleset according to review comments

a741dc8

oliverklee force-pushed the task/fixer-rules branch from 42b7359 to a741dc8 Compare June 29, 2024 08:06

JakeQZ reviewed Jun 30, 2024

View reviewed changes

Drop another rule for consistency

2e60b6d

oliverklee requested a review from JakeQZ June 30, 2024 15:30

JakeQZ approved these changes Jun 30, 2024

View reviewed changes

JakeQZ merged commit a8574c7 into main Jun 30, 2024
23 checks passed

JakeQZ deleted the task/fixer-rules branch June 30, 2024 22:38

JakeQZ mentioned this pull request Jul 7, 2024

[TASK] Add a fixer rule to ensure a PHP 8.4 compatibility #1271

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TASK] Update our coding standard #1264

[TASK] Update our coding standard #1264

oliverklee commented Jun 6, 2024

oliverklee commented Jun 12, 2024

oliverklee commented Jun 22, 2024

JakeQZ commented Jun 22, 2024

JakeQZ left a comment

JakeQZ Jun 22, 2024

oliverklee Jun 23, 2024

JakeQZ Jun 24, 2024

oliverklee Jun 29, 2024

JakeQZ left a comment

JakeQZ Jun 24, 2024

JakeQZ Jun 25, 2024

JakeQZ Jun 24, 2024

JakeQZ Jun 24, 2024

JakeQZ Jun 24, 2024

JakeQZ Jun 24, 2024

JakeQZ Jun 24, 2024

JakeQZ Jun 24, 2024

JakeQZ Jun 24, 2024

JakeQZ Jun 24, 2024

JakeQZ Jun 24, 2024

oliverklee commented Jun 29, 2024

JakeQZ left a comment

JakeQZ Jun 30, 2024

JakeQZ Jun 30, 2024

oliverklee Jun 30, 2024

JakeQZ left a comment

		'modernize_types_casting' => true,
		'no_short_bool_cast' => true,

[TASK] Update our coding standard #1264

[TASK] Update our coding standard #1264

Conversation

oliverklee commented Jun 6, 2024

oliverklee commented Jun 12, 2024

oliverklee commented Jun 22, 2024

JakeQZ commented Jun 22, 2024

JakeQZ left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JakeQZ left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliverklee commented Jun 29, 2024

JakeQZ left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JakeQZ left a comment

Choose a reason for hiding this comment