Skip to content

Fix closure indent issue and add tests#218

Merged
mbabker merged 9 commits intojoomla:masterfrom
photodude:patch-6
Jan 8, 2018
Merged

Fix closure indent issue and add tests#218
mbabker merged 9 commits intojoomla:masterfrom
photodude:patch-6

Conversation

@photodude
Copy link
Copy Markdown
Contributor

@photodude photodude commented Jan 7, 2018

This adds tests and fixes the closure indent issue in #206

Simple solution to check if the current token has a condition of being inside of a closure or anonymous class.

@photodude photodude changed the title Add closure indent test Fix closure indent issue and add tests Jan 8, 2018
@photodude
Copy link
Copy Markdown
Contributor Author

@mbabker @wilsonge, This now solves the closure issue and works with anonymous classes

@photodude photodude deleted the patch-6 branch January 8, 2018 02:10
@wilsonge
Copy link
Copy Markdown
Contributor

wilsonge commented Jan 8, 2018

Nice one!

@photodude
Copy link
Copy Markdown
Contributor Author

Thanks, I couldn't have done it if the PHPCS owner hadn't pointed me to the right function to figure out when we were in a closure so we could add more to the indent. I don't want to think about how many hours I spent over the past 20 vacation days to figure this out. I'm just glad it's done and we can move forward with preparing a stable release.

On a side note: PHPCS said they would consider a PR to add this control structure brackets sniff to the General standard since it's a common thing to put brackets on their own line.

@wilsonge
Copy link
Copy Markdown
Contributor

wilsonge commented Jan 8, 2018

On a side note: PHPCS said they would consider a PR to add this control structure brackets sniff to the General standard since it's a common thing to put brackets on their own line.

Nice if you're up for that would be really good to not have to maintain it ourselves :P

@photodude
Copy link
Copy Markdown
Contributor Author

@wilsonge see squizlabs/PHP_CodeSniffer#1847 for the PR

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

Successfully merging this pull request may close these issues.

3 participants