Skip to content

Fix codestyle after uptream release of cs-fixer #344

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

Merged
merged 1 commit into from
Nov 8, 2019

Conversation

rvanlaak
Copy link
Member

@rvanlaak rvanlaak commented Nov 6, 2019

The default CS-Fixer configurations for @Symfony has been updated.

See related comments from #342 (comment) until #342 (comment)

If wanted we can also add the related Makefile to this PR:

# Makefile

DIR := ${CURDIR}

phpstan:
	docker run --rm -v $(DIR):/project -w /project jakzal/phpqa:php7.1-alpine phpstan analyze

codestyle:
	docker run --rm -v $(DIR):/project -w /project jakzal/phpqa:php7.1-alpine php-cs-fixer fix

Copy link
Member

@welcoMattic welcoMattic left a comment

Choose a reason for hiding this comment

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

As we support PHP >= 7.1, we should prefer type hinting instead of PHPDoc. This PR could be the best place to fix it once for all, WDYT?

@rvanlaak
Copy link
Member Author

rvanlaak commented Nov 6, 2019

@welcoMattic that is exactly where #342 focuses on 👍

This PR is there to get master green again because of the upstream change to CS-Fixer.

Copy link
Member

@welcoMattic welcoMattic left a comment

Choose a reason for hiding this comment

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

LGTM 👏

@XWB
Copy link
Contributor

XWB commented Nov 6, 2019

You could also set no_superfluous_phpdoc_tags to false. I don't like that the doc comments are getting removed.

@rvanlaak
Copy link
Member Author

rvanlaak commented Nov 6, 2019

Same here, although I like that the lines of code gets reduced in this case. But, it is a bit hard to debate personal cs preference 😉

Imho following a standard cs set like the @Symfony we currently follow should be the leading choice, so other contributors can know what code style to expect.

Maybe the arguments for choosing @Symfony is something to document?

@XWB
Copy link
Contributor

XWB commented Nov 6, 2019

Imho following a standard cs set like the @symfony we currently follow should be the leading choice, so other contributors can know what code style to expect.

Sure, we could do that. But if the main purpose is to make master green again, setting no_superfluous_phpdoc_tags to false would achieve that too without introducing a new CS.

@rvanlaak
Copy link
Member Author

rvanlaak commented Nov 8, 2019

Merging this so @ker0x can get his PR #342 green again

@rvanlaak rvanlaak merged commit 820267c into php-translation:master Nov 8, 2019
@rvanlaak rvanlaak deleted the cs-fixer-codestyle branch November 8, 2019 09:50
@odolbeau odolbeau mentioned this pull request Nov 29, 2019
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