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

Re-enable disabled PHPMD rules #36

Closed
wants to merge 1 commit into from
Closed

Re-enable disabled PHPMD rules #36

wants to merge 1 commit into from

Conversation

thiemowmde
Copy link
Contributor

This is split from #31.

@@ -10,7 +10,12 @@
</rule>
<rule ref="rulesets/codesize.xml/TooManyMethods">
<properties>
<property name="maxmethods" value="20" />
<property name="maxmethods" value="16" />
Copy link
Collaborator

Choose a reason for hiding this comment

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

Considering this counts private methods, I rather not decrease this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we discussed this. In a small component like this it makes sense to lower such limits to the smallest number possible. If we ever want to add code that needs a slightly higher limit it's very easy (at least I hope it is) to increase this number in the same patch. It becomes part of the review then.

@JeroenDeDauw
Copy link
Collaborator

Inactive and needs to made to work with master

@JeroenDeDauw JeroenDeDauw deleted the composerMd branch April 16, 2018 23:06
@thiemowmde
Copy link
Contributor Author

Not that I mind very much, as I never found PHPMD's ruleset that helpful. But: when was PHPMD removed from master? What was the reasoning, where is the pull request that did it, and who approved it?

@manicki
Copy link
Member

manicki commented Apr 17, 2018

Briefly looking at the history it seems it has been dropped when moving up to PHP 7 as a minimum version (PHPMD didn't work well with PHP 7 or so), which I guess made sense.

@thiemowmde
Copy link
Contributor Author

And who approved it?

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

Successfully merging this pull request may close these issues.

3 participants