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

Fix closure indent issue and add tests #218

Merged
merged 9 commits into from
Jan 8, 2018
Merged

Conversation

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

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

@mbabker mbabker merged commit b09a00e into joomla:master Jan 8, 2018
@photodude photodude deleted the patch-6 branch January 8, 2018 02:10
@wilsonge
Copy link
Contributor

wilsonge commented Jan 8, 2018

Nice one!

@photodude
Copy link
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
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
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.

None yet

3 participants