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

Add more new sniffs #11

Merged
merged 1 commit into from
Dec 26, 2017
Merged

Add more new sniffs #11

merged 1 commit into from
Dec 26, 2017

Conversation

Majkl578
Copy link
Contributor

Second part of new sniffs, this time not 7.1-related, but more generic instead. Comments for each sniff are in the ruleset.
Most of these should already be ok on 99% of code base and are mostly to maintain consistence.
Also removed sniffs for spaces around !, replaced by generic sniff for spaces after ! (as agreed in past and suggested in #9).

cc @alcaeus @lcobucci

<!-- Require EOL at EOF -->
<rule ref="Generic.Files.EndFileNewline"/>
<!-- Forbid multiple classes per file -->
<rule ref="Generic.Files.OneClassPerFile"/>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one will be problematic for ORM where tests have their entities defined in the same file. Could be excluded on project level though.

Copy link
Member

Choose a reason for hiding this comment

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

Should be excluded per-directory IMO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If that is possible to configure, even better. 👍

Copy link
Member

Choose a reason for hiding this comment

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

It would simply require a custom local phpcs.xml file per directory to be inspected, with overrides for disabled rules

Copy link
Member

Choose a reason for hiding this comment

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

Isn't this covered by PSR1.Classes.ClassDeclaration.MultipleClasses (which is part of PSR-2)? If so, we already exclude things in the ORM

https://github.com/doctrine/doctrine2/blob/374e7ace49d864dad8cddbc55346447c8a6a2083/phpcs.xml.dist#L20-L22

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't realize it's already included 👍, we can remove these checks then, PSR-1 sniff provides check for classes/interfaces/traits at once.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed in 2ac121d.

@Ocramius Ocramius self-assigned this Dec 26, 2017
@Ocramius Ocramius added this to the 1.0.0 milestone Dec 26, 2017
Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@Ocramius Ocramius merged commit 166df1e into doctrine:master Dec 26, 2017
@Majkl578 Majkl578 deleted the more-sniffs branch December 27, 2017 01:08
@Majkl578 Majkl578 mentioned this pull request Dec 29, 2017
2 tasks
@Majkl578 Majkl578 modified the milestones: 1.0.0, 1.1.0 Dec 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants