added whitelisting

[Cielquan]

Multiple issues with false positives motivated me to add a whitelisting feature.

You can extend the default whitelist or overwrite it from command line.
Whitelist entries are interpreted as regex.

WHITELIST = [
    r'pylint',
    r'pyright',
    r'(?i)noqa',
    r'type:\s*ignore',
    r'fmt:\s*(on|off)',
    r'TODO',
    r'FIXME',
    r'XXX'
]

Currently the regex check is case sensitive. A single entry can be made case insensitive by adding (?i) like above for noqa.

Maybe case insensitivity by default is okay?
What other defaults do you know that should be added?

I'm currently working on the tests and will also update the README afterwards.

Closes #19 PR for manually ignoring TODO which is covered by this PR

Fixes #25
Fixes #24
Fixes #16
Fixes #15
Fixes #11
Resolves #26

EDIT: Please see comment(s) below

[coveralls]

Coverage remained the same at 100.0% when pulling 67dd2df on Cielquan:master into 9859531 on myint:master.

[Cielquan]

First I introduced the whitelist feature with a global variable but could not find a way of testing the manipulation of the glob var. Then I put all the functions and glob vars except __version__ and main() into a Eradicator class so there is a shared context for the functions. I hope my invasive change in the code is okay.

The whitelist check is case insensitive by default.

I updated all test cases with the new class structure and added tests for my added functionality.

I also added a description to the README for how to use the whitelisting feature.

If you merge this PR please release the change as version 2.0 as the class structure is backwards incompatible and will break e.g. flake8-eradicate.

[myint]

Thanks! I'll let @sobolevn review this.

[sobolevn]

Hi! Let's start with the most important question:

Do we really need to refactor the public API of this project?

I am asking because, this would also break stuff that relies on eradicate, like flake8-eradicate: https://github.com/sobolevn/flake8-eradicate/blob/master/flake8_eradicate.py#L103

What are the reasons behind this refactor?

[Cielquan]

Hi! Let's start with the most important question:

Do we really need to refactor the public API of this project?

I am asking because, this would also break stuff that relies on eradicate, like flake8-eradicate: https://github.com/sobolevn/flake8-eradicate/blob/master/flake8_eradicate.py#L103

What are the reasons behind this refactor?

Ok I tried to capture my thoughts from around 2 months ago in why I did the API change:

First I tried to implement the whitelist feature with the current API. I thought of using glob variables containing the whitelist stuff and change them when the user changes the default whitelist (with new args: --whitelist, --whitelist-extend). Well but changing glob vars is not that nice though. Especially when the module gets used by third parties (I had flake8-eradicate in mind there).

Another thought was to init a variable containing the whitelist and pass it form the main() function to the function which uses it. But this would also be problematic with third parties like flake8-eradicate which does not use the main() function. And I would also have to haul the variable through multiple functions who just pass it to the next function.

So the problem was the context which needs to be shared. For that reason I put the functions into a class.

Honestly I did not change that much. When you compare the two file versions side by side you can see that I only did the "class-rewrite", added two args for CLI and the the whitelist-regex-feature.

The API is also pretty easy to update:
Instead of calling filter_commented_out_code directly you need to import and init the Eradicator class. Then you can, if you add the whitelist args to flake8-eradicate also, update the whitelist in the class-instance and finally call filter_commented_out_code.

IMO this change is for the better and would also come in handy for potential future additions.

[sobolevn]

Ok, fair enough! I will proceed to review our code than! 👍

[Cielquan]

If you want me to I could make the PR for flake8-eradicate (when this is merged).

If you have other questions please feel free to ask 😉

[sobolevn]

First round is done! 👍

[Cielquan]

Fixed stuff mentioned in your review .. 2 points are open .. see above

[Cielquan]

All requested changes are done

[sobolevn]

Awesome! There's almost everything fixed!

[Cielquan]

Moved all regex to class const

One question remains .. see above

[sobolevn]

Since it is the first PR I merge, I kindly ask @myint to review it as well.
If he does not have the time, I will merge this in a week (1.09)

[patrick91]

@sobolevn any update on this? We want to introduce flake8-eradicate to our project and this is blocking us 😊

[sobolevn]

Merged!

Now, we would have to figure out how to release it! 😆

[sobolevn]

@myint there are three options:

1. You can setup automatic publishing of packages in travis ci. Each time new tag is published - a package is released automatically
2. You can add me to PyPI to make manual release
3. You can release package by yourself, but we will need to ping you to do that from time to time

Which one does look the best to you?

[myint]

Thanks! I've chosen option 2 for expediency. Feel free to enable automatic publishing as needed. I don't think I've set that up in any of my github.com repositories before.

[Cielquan]

@sobolevn Will you release the change as version 2.0.0 ?

[sobolevn]

Yes, next week. Thanks for the reminder! 👍

[johnthagen]

You can add me to PyPI to make manual release

@sobolevn Please also see #30 where it would be great if this package published wheels as well to PyPI.