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

Code cleaning #379

Merged
merged 1 commit into from Jan 20, 2017

Conversation

Projects
None yet
3 participants
@jaymoulin

jaymoulin commented Jun 24, 2016

No description provided.

@@ -28,3 +28,5 @@ build.xml export-ignore
phpunit.xml.dist export-ignore
pdepend.xml.dist export-ignore
composer.lock export-ignore
.editorconfig export-ignore
appveyor.yml export-ignore

This comment has been minimized.

@ravage84

ravage84 Jun 24, 2016

Member

Merged that separately, thanks.

This comment has been minimized.

@jaymoulin

jaymoulin Jun 24, 2016

IMHO, we should do something with .gitignore also to remove all IDE cases because they should be in the global .gitignore from contributors. But what about the setup folder?

This comment has been minimized.

@ravage84

ravage84 Jun 24, 2016

Member

While I agree that such folders should be listed in the global .gitignore file of contributors, the reality is, they aren't and in th end it's easier to have a few entry in each repo's .gitignore file.

By the way, I have no global .gitignore file myself. 😼

@@ -82,15 +83,15 @@ public function apply(AbstractNode $node)
}
}
private function isElseScope($scope, $parent)
private function isElseScope($scope, ASTNode $parent)

This comment has been minimized.

@ravage84

ravage84 Jun 24, 2016

Member

Ths changes the interface, which could break BC (for extending third party classes).

This comment has been minimized.

@jaymoulin

jaymoulin Jun 24, 2016

Third party classes cannot extends this method since it's private and the code in it requires $parent to be an ASTNode instance. Maybe the purpose of this method is to be protected?

This comment has been minimized.

@ravage84

ravage84 Jun 24, 2016

Member

True, didn't see it's a private.
Nonetheless, I let @manuelpichler have a look.

@@ -87,20 +87,20 @@ public function apply(AbstractNode $node)
}
}
private function isStaticMethodCall($methodCall)
private function isStaticMethodCall(AbstractNode $methodCall)

This comment has been minimized.

@ravage84

ravage84 Jun 24, 2016

Member

Same as above.

This comment has been minimized.

@jaymoulin

jaymoulin Jun 24, 2016

same as above ;)

@@ -77,6 +77,7 @@ public function apply(AbstractNode $node)
if ($node->getMetric('nom') <= $threshold) {
return;
}
/** @var $node AbstractTypeNode */

This comment has been minimized.

@ravage84

ravage84 Jun 24, 2016

Member

Not sure, if I like to clutter the code will all these annotations...

*
* @return array
* @return array|null

This comment has been minimized.

@ravage84

ravage84 Jun 24, 2016

Member

Can you please PR the changes to getIgnorePattern() without the @var annotation separately?

@ravage84

This comment has been minimized.

Member

ravage84 commented Jun 24, 2016

Thanks for the contributions.

To make merging easier in the future, please open separate PRs.
Fo example all @var annotation changes in one PR.
So, if we reject this one for example, the other PRs can be merged without adjustment.

@ravage84 ravage84 added this to the 2.4.4 milestone Jun 24, 2016

@jaymoulin

This comment has been minimized.

jaymoulin commented Jun 24, 2016

Thanks for your return. I'll manage to handle that

@jaymoulin jaymoulin force-pushed the jaymoulin:code-clean branch from 886a9e6 to 902da6c Jun 24, 2016

@manuelpichler manuelpichler merged commit bb57b00 into phpmd:master Jan 20, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jaymoulin jaymoulin deleted the jaymoulin:code-clean branch Jan 20, 2017

@ravage84 ravage84 modified the milestones: 2.5.1, 2.6.0 Jan 30, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment