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 CLI IgnoreViolationsOnExit option flag #380

Merged
merged 3 commits into from Jun 28, 2016

Conversation

jaymoulin
Copy link
Contributor

add --ignore-exit-violations CLI flag to allow exit code 0 event if violation is found (will allow successfull build for CI)

…iolation is found (will allow successfull build for CI)
@ravage84 ravage84 added this to the 2.5.0 milestone Jun 24, 2016
@ravage84
Copy link
Member

PHPCS has a similar cli switch:
https://github.com/squizlabs/PHP_CodeSniffer/wiki/Configuration-Options#ignoring-errors-when-generating-the-exit-code

$ phpcs --config-set ignore_errors_on_exit 1
$ phpcs --config-set ignore_warnings_on_exit 1

So, I would prefer the switch to be --ignore-violations-on-exit.
It's also more self-explanatory.

@ravage84
Copy link
Member

@ravage84
Copy link
Member

ravage84 commented Jun 24, 2016

*
* @return boolean
*/
public function hasIgnoreExitViolations()
Copy link
Member

Choose a reason for hiding this comment

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

Please rename to ignoreExitViolations().
Haven't looked into it, but does it need to be public?
If not, please use protected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Method is used in src/main/php/PHPMD/TextUI/Command.php:132

@ravage84
Copy link
Member

@jaymoulin many thanks for your contribution. This is certainly useful.

Do you see a chance for adding a unit test, too?

@jaymoulin
Copy link
Contributor Author

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

@@ -99,4 +99,29 @@ public function testMainReturnsViolationExitCodeForSourceWithNPathViolation()
);
$this->assertEquals(Command::EXIT_VIOLATION, $exitCode);
}

/**
* testMainReturnsViolationExitCodeForSourceWithNPathViolation
Copy link
Member

@ravage84 ravage84 Jun 27, 2016

Choose a reason for hiding this comment

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

Copy paste error, better explain the meaning of the test instead of copy pasting the name of the function/method.
E.g.:
Tests if main returns success Exit Code for Source with NPath Violation and IgnoreViolationsOnExit Flag

This kind of documentation is meant for a developer, not for a computer... 😼

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 thought it was a Lead Developper choice so I just followed the testcase coding style

@ravage84
Copy link
Member

Apart from my nitpicking, this really looks good.
Thanks again for your work!

@ravage84 ravage84 changed the title Can Ignore Violation exitcode Add CLI IgnoreViolationsOnExit option flag Jun 27, 2016
@ravage84 ravage84 merged commit 3d7a307 into phpmd:master Jun 28, 2016
@jaymoulin jaymoulin deleted the exit-code branch October 3, 2016 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

None yet

2 participants