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

Adding Line Numbers To Mutator Ignore List #663

Merged
merged 3 commits into from Mar 16, 2019

Conversation

Fenikkusu
Copy link
Contributor

@Fenikkusu Fenikkusu commented Mar 10, 2019

This PR:

This started off as adding a 'falsePositives' feature to the config, but then I saw the ignore stuff in the json schema. Good to know. The only shortcoming I see is that there wasn't the ability to specify line numbers specifically. IE: Ignore this call in this function on line 123 but process call in the same function on line 456. This PR alters the logic to allow for line numbers in the search string as well.

IE:

    "mutators": {
        "MethodCallRemoval": {
            "ignore": [
                "Infection\\Finder\\SourceFilesFinder::__construct::63"
            ]
        }
    }

Note: The config change to infection.json.dist will fix the testing in PR #660.

Copy link
Member

@maks-rafalko maks-rafalko left a comment

Choose a reason for hiding this comment

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

Thanks, looks good to me.

The only one thing that I warn about is that line numbers can change quite often, resulting to not up-to-date ignore config. Related to #213

@Fenikkusu
Copy link
Contributor Author

The way I see it, this is more for false positives than anything. While it is true that they could change frequently, the idea here is to mark items that likely don't.

"mutators": {
"MethodCallRemoval": {
"ignore": [
"Infection\\Finder\\SourceFilesFinder::__construct::63"
Copy link
Member

Choose a reason for hiding this comment

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

This is for parent's __construct. It does some things, but they're quite hard to test, and that is kind of out of scope of our project.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's the reason I put it in there. Granted, I do have to disagree to some point. It is quite easy to test. I had already commented the test code in #660. The problem is that you have to test using reflection, something most people tend to frown upon.

Copy link
Member

@sanmai sanmai Mar 11, 2019

Choose a reason for hiding this comment

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

Yeah, true. What do you think about adding this rationale to the PR description? It is not immediately obvious, that why I had to comment.

(It's kind of award more than just hard to test this without reflection.)

@maks-rafalko maks-rafalko added this to the 0.13.0 milestone Mar 16, 2019
@maks-rafalko
Copy link
Member

Thank you @Fenikkusu

@maks-rafalko maks-rafalko merged commit 334983e into infection:master Mar 16, 2019
maks-rafalko added a commit that referenced this pull request Mar 25, 2019
maks-rafalko added a commit that referenced this pull request Mar 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants