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

DisallowShortTernary: improve docs #1800

Merged
merged 1 commit into from Sep 22, 2019

Conversation

jrfnl
Copy link
Member

@jrfnl jrfnl commented Sep 20, 2019

The valid example line was a bit too long... oops...

The valid example line was a bit too long... oops...
@jrfnl
Copy link
Member Author

jrfnl commented Sep 20, 2019

Before:
image

After:
image

Copy link
Member

@dingo-d dingo-d left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't have a rule about where the break for the conditional should be, right?

What I mean is:

$height = ! empty( $data['height'] ) ?
    $data['height'] : 0;

vs

$height = ! empty( $data['height'] ) ?
    $data['height'] :
    0;

(would probably be outside of the scope of this sniff anyhow I guess)

@jrfnl
Copy link
Member Author

jrfnl commented Sep 20, 2019

We don't have a rule about where the break for the conditional should be, right?

Correct, no such rule currently exists for Core or otherwise.

If we would create such a rule, I would advocate for it to be a separate sniff and to sniff for all multi-line statements with an operator at the end/start of a line.

My personal preference if such a rule would be suggested, would be to always have the operator at the start of the next line (and yes, I know the above example does not comply and in a way, ternaries may be the exception anyway).

Reasoning behind that preference: for longer multi-line statements, this will often make for smaller and cleaner diffs and it improves readability as the operator tells you how the code directly after it will be handled, instead of having to "switch lines" with your eyes.

Some examples:

// If the second or third line would be removed, only that line will show in the diff.
if ( $a === $b
    && $b < $c
    || $c !== $d
) { ... }

// Readability
$var = 10
    + 20
    . 'text';

// versus
$var = 10 +
    20 .
    'text';

Anyways, not something which should be handled in this PR either way.

If you want to propose such a sniff, I suggest opening an issue for it.

@GaryJones GaryJones merged commit 484335b into develop Sep 22, 2019
@GaryJones GaryJones deleted the feature/disallow-short-ternary-improve-docs branch September 22, 2019 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants