php-fig / per-coding-style

PER coding style
262 stars 26 forks source link

Match Expressions operator alignment #92

Open jacobcassidy opened 2 months ago

jacobcassidy commented 2 months ago

Currently, there are no coding style rules for match expression operators. I suggest having the double-arrow operators in alignment, with the option of breaking out of alignment when using more than one conditional expression.

For example, this is not aligned:

$asset_dir = match ($asset_type) {
    'style' => 'build/base/css',
    'script' => 'build/base/js',
    'editor_ui_style' => 'build/blocks/css',
    'editor_ui_script' => 'build/blocks/js',
    default   => 'build/base'
};

The desired alignment:

$asset_dir = match ($asset_type) {
    'style'            => 'build/base/css',
    'script'           => 'build/base/js',
    'editor_ui_style'  => 'build/blocks/css',
    'editor_ui_script' => 'build/blocks/js',
    default            => 'build/base'
};

An example of breaking out of alignment with multiple conditional expressions for a match:

$asset_dir = match ($asset_type) {
    'style'            => 'build/base/css',
    'script'           => 'build/base/js',
    'editor_ui_style'  => 'build/blocks/css',
    'editor_ui_script' => 'build/blocks/js',
    'expession_1', 'expression_2', 'expression_3' => 'some value',
    default            => 'build/base'
};
vjik commented 2 months ago

If add rule for that case, then need add rule for arrays also. And they must be consistent. But I like array/match code without alignment…

jrfnl commented 2 months ago

The rule could also be along the lines of the below:

For multi-line arrays, keyed lists and match control structures: Either align all arrows to the widest key (match: case expression) or don't align at all and always have one space on either side of the double arrow. Mixing aligned entries and non-aligned entries in the same structure is not allowed.

Crell commented 2 months ago

I can see why this approach is appealing, but as with arrays, it's also a lot of additional maintenance with a lot of edge cases. I could see an argument for "do X or Y", but I worry that could cause issues for SA tools that need to figure out which approach someone is using. (cc @jrfnl @ondrejmirtes for their input.)

I do agree that whatever it is should be consistent with arrays. Similarly, we may want to consider specifying a multi-line style, like short-lambdas already have. That would be something like:

$asset_dir = match ($asset_type) {
    'style' 
            => 'build/base/css',
    'script' 
            => 'build/base/js',
    'editor_ui_style' 
            => 'build/blocks/css',
    'editor_ui_script' 
            => 'build/blocks/js',
    default   
            => 'build/base'
};

(Assuming each arm was long enough to break to multiple lines, which this example isn't but you get the idea.)

jrfnl commented 2 months ago

I could see an argument for "do X or Y", but I worry that could cause issues for SA tools that need to figure out which approach someone is using.

It would definitely make things more complicated, but is doable IMO. For alignment, you'd need to loop through the structure twice anyway, first time to figure out the widest key/case, second time to check the alignment against the expected alignment.

In that first loop you could also set a flag for align/don't align based on: if the space between the key and the arrow is more than 1 and this results in the same arrow position as the arrow for either the previous or the next item, then (some) alignment is found, so align the arrows, otherwise don't.

Similarly, we may want to consider specifying a multi-line style, like short-lambdas already have.

That one IMO is more complicated as it raises more questions:

It also raises questions about the indentation, in particular about the indentation for multi-line array values: should this always be "key indent + tab-width" for the next line ? Or should it be "take key indent as a minimum" ? Or first line "key indent + tab-width", subsequent lines "key indent + tab-width x 2" ? Or ... ?

Crell commented 2 months ago

From section 7.1:

The expression portion MAY be split to a subsequent line. If so, the => MUST be included on the second line, and MUST be indented once.

That section is for short-closures, but my point is that if we mention multi-line breaks for match() at all, it should follow suit for consistency.

jrfnl commented 2 months ago

@Creel I hear what you are saying, but then we get a discussion on when a value should be considered multi-line.

Think:

$array = [
    'sub-array' => [
        'item1',
        'item2',
    ],
    'closure' => function($a) {
        return $a * 10;
    },
    'long string' => 'very long line'
        . 'second part of very long line',
];

In all of the above situations, I would find it counter-intuitive (and fugly) to have the double arrow on the next line and have the value indented an additional level.