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

Projects
None yet
2 participants
@jaymoulin

jaymoulin commented Jun 24, 2016

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

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

@ravage84 ravage84 added the Feature label Jun 24, 2016

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

@ravage84

This comment has been minimized.

Member

ravage84 commented Jun 24, 2016

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

This comment has been minimized.

@ravage84

This comment has been minimized.

Member

ravage84 commented Jun 24, 2016

*
* @return boolean
*/
public function hasIgnoreExitViolations()

This comment has been minimized.

@ravage84

ravage84 Jun 24, 2016

Member

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

This comment has been minimized.

@jaymoulin

jaymoulin Jun 24, 2016

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

@@ -438,7 +458,9 @@ public function usage()
'--exclude: comma-separated string of patterns that are used to ' .
'ignore directories' . \PHP_EOL .
'--strict: also report those nodes with a @SuppressWarnings ' .
'annotation' . \PHP_EOL;
'annotation' . \PHP_EOL .
'--ignore-exit-violations: will exit with 0 even if a ' .

This comment has been minimized.

@ravage84

ravage84 Jun 24, 2016

Member

--ignore-violations-on-exit: will exit with a zero code, even if any violations are found

@ravage84

This comment has been minimized.

Member

ravage84 commented Jun 24, 2016

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

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

@jaymoulin

This comment has been minimized.

jaymoulin commented Jun 24, 2016

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

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

This comment has been minimized.

@ravage84

ravage84 Jun 27, 2016

Member

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... 😼

This comment has been minimized.

@jaymoulin

jaymoulin Jun 27, 2016

I thought it was a Lead Developper choice so I just followed the testcase coding style

* @group phpmd
* @group phpmd::textui
* @group unittest
*/

This comment has been minimized.

@ravage84

ravage84 Jun 27, 2016

Member

Please add a @see ::testMainReturnsViolationExitCodeForSourceWithNPathViolation Same as the other test, but with '--ignore-violations-on-exit' set.

*
* @return boolean
*/
public function hasIgnoreExitViolations()
public function ignoreExitViolations()
{
return $this->ignoreExitViolations;

This comment has been minimized.

@ravage84

ravage84 Jun 27, 2016

Member

Rename to ignoreViolationsOnExit please.

*
* @return boolean
*/
public function hasIgnoreExitViolations()
public function ignoreExitViolations()

This comment has been minimized.

@ravage84

ravage84 Jun 27, 2016

Member

On a secont thought, after we renamed the CLI option, we should rename this appropriately, too.

Please rename to ignoreViolationsOnExit().

🙇

@@ -200,6 +200,45 @@ public function testCliOptionsAcceptsVersionArgument()
}
/**
* testIgnoreExitViolationsReturnsFalseByDefault

This comment has been minimized.

@ravage84

ravage84 Jun 27, 2016

Member

Personally, I don't like it, when people just cpy and paste it.

Why not say what it does, like:
Tests if ignoreViolationsOnExit returns false by default

This comment has been minimized.

@jaymoulin

jaymoulin Jun 27, 2016

I thought it was a Lead Developper choice so I just followed the testcase coding style

This comment has been minimized.

@ravage84

ravage84 Jun 28, 2016

Member

It is or better said was. But it's never too late to start improving 😼

*
* @return void
*/
public function testIgnoreExitViolationsReturnsFalseByDefault()

This comment has been minimized.

@ravage84

ravage84 Jun 27, 2016

Member

Rename to testIgnoreViolationsOnExitReturnsFalseByDefault() please.

@ravage84

This comment has been minimized.

Member

ravage84 commented Jun 27, 2016

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

@ravage84 ravage84 changed the title from Can Ignore Violation exitcode to Add CLI IgnoreViolationsOnExit option flag Jun 27, 2016

@ravage84 ravage84 merged commit 3d7a307 into phpmd:master Jun 28, 2016

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:exit-code branch Oct 3, 2016

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