Skip to content

Engine accepts paths to custom ruleset #6

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 3 commits into from
Dec 7, 2015

Conversation

ABaldwinHunter
Copy link
Contributor

When PHPMD gets run, it accepts rule names, e.g.
codesize,unusedcode,naming

as a comma separated string.

It also accepts paths to a custome rule defined by the user mixed
in. For example:

  phpmd:
    enabled: true
    config:
      rulesets: "unusedcode,codesize,cleancode,design,naming,phpmd.xml"

where phpmd.xml contains a custom rule set up by user.

Previously, our engine failed to honor that config setup because it didn't
prefix /code/ to the path. The result was 0 issues found across board.

This change fixes that behavior.

cc @codeclimate/review @leftees

foreach ($configRulesets as &$r) {
if (!in_array($r, $officialPhpRulesets)) {
$r = "/code/$r";
continue;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this actually needed? There's nothing else in the loop after this, so I don't think it accomplishes anything.

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 you're right, I don't think it's needed. I can remove

When PHPMD gets run, it accepts rule names, e.g.
codesize,unusedcode,naming

as a comma separated string.

It also accepts paths to a custome rule defined by the user mixed
in. For example:

```
  phpmd:
    enabled: true
    config:
      rulesets: "unusedcode,codesize,cleancode,design,naming,phpmd.xml"
```
where `phpmd.xml` contains a custom rule set up by user.

Previously, our engine failed to honor that config setup because it didn't
prefix `/code/` to the path. The result was 0 issues found across board.

This change fixes that behavior.
@ABaldwinHunter
Copy link
Contributor Author

Thanks @wfleming. Updated. Ready for another look!


foreach ($configRulesets as &$r) {
if (!in_array($r, $officialPhpRulesets)) {
$r = "/code/$r";
Copy link
Contributor

Choose a reason for hiding this comment

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

💄 looks like this needs to be indented another column?

@wfleming
Copy link
Contributor

wfleming commented Dec 7, 2015

Two more suggestions (one of which PHP code sniffer also caught, so that's working, which is nice.). Other than that, LGTM.

@ABaldwinHunter
Copy link
Contributor Author

Thanks @wfleming.

I made a couple of minor style fixes in that file as well.


foreach ($configRulesets as &$r) {
if (!in_array($r, $officialPhpRulesets)) {
$r = "/code/$r";
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 it might be worth it to detect the case where someone enters an absolute path (ie. don't prepend /code if the string starts with /)

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 a user provides an absolute path to ruleset file, we
won't prepend `/code/`. This behavior seems intuitive and also
prevents breakage with current codeclimate-php configs which may have
come to use `/code` as a workaround.
@ABaldwinHunter
Copy link
Contributor Author

@gordondiggs @wfleming updated. Ready for a (hopefully) final look!

@wfleming
Copy link
Contributor

wfleming commented Dec 7, 2015

👍

ABaldwinHunter pushed a commit that referenced this pull request Dec 7, 2015
Engine accepts paths to custom ruleset
@ABaldwinHunter ABaldwinHunter merged commit 166ca95 into master Dec 7, 2015
@ABaldwinHunter ABaldwinHunter deleted the abh-custom-rulesets branch December 7, 2015 21:37
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