-
Notifications
You must be signed in to change notification settings - Fork 56
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
Add Generic.Arrays.ArrayIndent sniff #54
Add Generic.Arrays.ArrayIndent sniff #54
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it 👍
Just some comments before you have my approval 😄
lib/Doctrine/ruleset.xml
Outdated
@@ -8,6 +8,8 @@ | |||
<exclude name="PSR2.Namespaces.UseDeclaration.SpaceAfterLastUse"/> | |||
</rule> | |||
|
|||
<!-- Indent array keys with 4 spaces --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe "Force array indentation"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
7, | ||
8, | ||
9, | ||
], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update the tests with a example with string
s in the keys, like:
$foo = [
'foo' => [
'bar' => 2,
1 => 'baz',
];
];
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this is also an interesting scenario:
$items->append($one)
->append(
[
0,
1,
2,
...,
14,
15,
16,
]
);
And I'd expect to have something like:
$items->append($one)
->append(
[
0,
1,
2,
...,
14,
15,
16,
]
);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lcobucci that interferes with another rule:
Multi-line function call not indented correctly; expected 10 spaces but found 0
In the end an array initializer is an array initializer, is it not? We could add tests for all kinds of scenarios, but that doesn't change the array tokens as such. I would like to avoid possible conflicts with other rules as much as possible for now and in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@carusogabriel done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@deeky666 that makes sense =)
1251e0f
to
b85e21a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
lib/Doctrine/ruleset.xml
Outdated
@@ -17,6 +19,7 @@ | |||
<!-- But allow empty catch --> | |||
<exclude name="Generic.CodeAnalysis.EmptyStatement.DetectedCatch"/> | |||
</rule> | |||
<rule ref="Generic.CodeAnalysis.ForLoopShouldBeWhileLoop"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this rule?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh sry, that was something else I accidently commited xD. will fix it
b85e21a
to
fa4d8f8
Compare
Seems this is not part of PSR-2 -.-