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

Generic/MultipleStatementAlignment: fix bug/list syntax before assignment operator #1924

Merged

Conversation

jrfnl
Copy link
Contributor

@jrfnl jrfnl commented Mar 6, 2018

This simplifies the fixes which were previously committed to fix #1870 and #1848, which were released in 3.2.3.

Both those fixes try to prevent the sniff getting confused over (potentially) multi-line value assignments, such as closures and arrays.

The simplification removes the previously added code in favour of skipping to the end of a statement as soon as an assignment operator has been found.

This might also make the sniff marginally faster as the tokens between the assignment operator and the end of the statement will no longer be examined.

N.B.: The reason for going one back from the end of the assignment (line 241), is that otherwise the code addressing the issue covered by the unit test on line 220 / added in commit 579d39c would not be triggered, leading to incorrect results.

Fixes #1922

Side-note: I think it would be a good idea to add unit tests for the File::findEndOfStatement() method.

@linaori
Copy link

linaori commented Mar 6, 2018

Would it be an idea to add this fixture as well, just to show that if there is no blank line, it should still be aligned?

public function buildForm(FormBuilderInterface $builder, array $options)
{
    $transformer                                    = new ContractTransformer($options['contracts']);
    $types                                          = ['support.contact.question' => ContactData::QUESTION];
    [$important, $questions, $incidents, $requests] = $this->createContractBuckets($options['contracts']);
}

…ment operator

This simplifies the fixes which were previously committed to fix 1870 and 1848, which were released in `3.2.3`.

Both those fixes try to prevent the sniff getting confused over (potentially) multi-line value assignments, such as closures and arrays.

The simplification removes the previously added code in favour of skipping to the end of a statement as soon as an assignment operator has been found.

This might also make the sniff marginally faster as the tokens between the assignment operator and the end of the statement will no longer be examined.

N.B.: The reason for going one back from the end of the assignment, is that otherwise the code addressing the unit test on line 220 / added in commit 579d39c would not be triggered, leading to incorrect results.

Side-note: I think it would be a good idea to add unit tests for the `File::findEndOfStatement()` method.
@jrfnl jrfnl force-pushed the feature/1922-assignment-alignment branch from f48a413 to 7fb368d Compare March 6, 2018 14:04
@jrfnl
Copy link
Contributor Author

jrfnl commented Mar 6, 2018

Would it be an idea to add this fixture as well, just to show that if there is no blank line, it should still be aligned?

I've added the additional unit test as a safe-guard.

Just so you know: that test case would not be affected by the fix I've now implemented anyway as the whole "check for arrays" bit has been removed, so this is more a safeguard for the future than that it actually tests something now.

@linaori
Copy link

linaori commented Mar 6, 2018

Fair enough! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants