Skip to content

Conversation

@egeloen
Copy link
Contributor

@egeloen egeloen commented Jun 27, 2014

This PR rewrites a little bit the way how comments are ignored in order to exclude all lines without real executable codes.

Before:

before

After:

after

Additionally, this PR fixes #233

@sebastianbergmann
Copy link
Owner

I cannot reproduce this:

screenshot from 2014-06-28 05 57 27

Or is this only a problem when the file is not executed?

@egeloen
Copy link
Contributor Author

egeloen commented Jun 28, 2014

@sebastianbergmann Yes, it is when the file is not executed.

@egeloen
Copy link
Contributor Author

egeloen commented Jun 28, 2014

Basically, the purpose of this PR is to rationalize how comments are ignored. A comment starting with // or /* should be handled/ignored the same way in a code coverage context whether the code is executed or not.

@sebastianbergmann
Copy link
Owner

Can you adapt your patch for master and send a separate pull request for that? I'll merge them both then. Thanks!

@egeloen egeloen changed the title Ignore comments starting by '/*' or splited on multiple lines [2.0] Ignore comments starting by '/*' Jun 29, 2014
@egeloen
Copy link
Contributor Author

egeloen commented Jun 29, 2014

I have replaced if ($token instanceof PHP_Token_DOC_COMMENT || ($token instanceof PHP_Token_COMMENT && (0 === strpos($_token, '/*')))) { by if (0 === strpos($_token, '/*')) { as we are in a switch where we only deal with PHP_Token_DOC_COMMENT and PHP_Token_COMMENT :)

Additionally, I have merged the last two if conditions in a single one.

@egeloen
Copy link
Contributor Author

egeloen commented Jun 29, 2014

@sebastianbergmann Should I backport it to 1.2 too?

sebastianbergmann added a commit that referenced this pull request Jun 30, 2014
Ignore comments starting by '/*'
@sebastianbergmann sebastianbergmann merged commit 09a53c2 into sebastianbergmann:2.0 Jun 30, 2014
@egeloen egeloen deleted the comment-coverage branch June 30, 2014 10:26
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