Skip to content

Conversation

@carusogabriel
Copy link

I'm not sure if we and to add all new Snifss.

{
$message = '';
foreach ($errorsForFile as $line => $errorsForPossition) {
foreach ($errorsForFile as $line) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this changes the behaviour of the code - see https://3v4l.org/XTpc9

Copy link
Author

Choose a reason for hiding this comment

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

I believe we should not look for variables at foreachs than.

/cc @kukulich

Copy link
Contributor

Choose a reason for hiding this comment

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

The right fix is “foreach (array_keys($errorsForFile) as $line)”

@VasekPurchart
Copy link
Member

VasekPurchart commented Aug 28, 2018

Thank you for the PR, but I cannot accept it now, because of the stability promise of this package - not introducing sniffs which are not covered by the written standard. I would have to release next major version which I am not ready for atm - need to add more things.

The last version of Slevomat CS has some great sniffs in it, so I had already made notes to add them to the standard in the next major, here is a temp list I made:

  • SlevomatCodingStandard.Classes.ModernClassNameReference
  • SlevomatCodingStandard.Functions.StaticClosure
  • SlevomatCodingStandard.Operators.RequireCombinedAssignmentOperator
  • SlevomatCodingStandard.TypeHints.NullTypeHintOnLastPosition
  • SlevomatCodingStandard.Classes.TraitUseDeclaration
  • SlevomatCodingStandard.Classes.TraitUseSpacing
  • SlevomatCodingStandard.PHP.UselessSemicolon

Then there is the issue of sniffs, which are not really covering coding standard per se, but are more of static analysis nature (like UnusedVariable etc.). These are mentioned in the readme, but I don't think it is desirable to add them in a minor version again. In fact I was thinking about this and it might be even better to separate all these sniffs into separate ruleset (and then maybe create a combined one) - or some other option like that - I will have to investigate it before the next major release. These sniffs are of course useful, but can be redundant to tools like PHPStan etc. + PHPCS will be always more limited in this area. So this is great if you can't use it somewhere else, but I don't think they are in the same area as the other "really" coding standard sniffs, so it is better to give a choice to the end users to either use them or not, but not "force" it on them.

I have prepared a new PR with new Slevomat sniffs, which are covering the existing standard: #50

@carusogabriel
Copy link
Author

@VasekPurchart Thank you for this full explanation 😊

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.

4 participants