Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Match Expressions operator alignment #92

Open
Tracked by #97
jacobcassidy opened this issue Aug 10, 2024 · 6 comments
Open
Tracked by #97

Match Expressions operator alignment #92

jacobcassidy opened this issue Aug 10, 2024 · 6 comments

Comments

@jacobcassidy
Copy link

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'
};
@jacobcassidy jacobcassidy changed the title Add Match Expressions operator alignment Match Expressions operator alignment Aug 10, 2024
@vjik
Copy link
Contributor

vjik commented Aug 10, 2024

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
Copy link
Contributor

jrfnl commented Aug 10, 2024

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
Copy link
Collaborator

Crell commented Aug 10, 2024

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
Copy link
Contributor

jrfnl commented Aug 10, 2024

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:

  • Should the new line be before the arrow or after ? Or are both allowed ?
  • If both allowed, should consistency be enforced within a structure ? I.e. either always have the new line before the arrow or always after (within the same structure).
  • If one entry is multi-line, should all entries be multi-line ? Or should there basically be a setting allowNewLines=true and should new lines be ignored when checking the alignment ?

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
Copy link
Collaborator

Crell commented Aug 11, 2024

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
Copy link
Contributor

jrfnl commented Aug 11, 2024

@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.

@Crell Crell mentioned this issue Sep 3, 2024
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants