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

Improve MeFilter #271

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

ukleinek
Copy link
Contributor

Note this is completely untested, so please look deeply before merging :-)

The me_tag option does nothing that couldn't be done with the tags option
that works on all Filters.

Recommend using tags instead of me_tag in the documentation and drop the
custom handle_message. Add some code to make existing rules work as
expected.

As a side-effect this makes

	[MeFilter]
	tags = -something

work as expected. It used to do nothing, now it removes the something tag.
Instead of accepting only tags_blacklist just pass everything but
me_tags to the super class's init function. This way the message can be
overwritten in the config which up to now yields an exception:

	TypeError: __init__() got an unexpected keyword argument 'message'
@codecov
Copy link

codecov bot commented May 27, 2020

Codecov Report

Merging #271 into master will decrease coverage by 0.10%.
The diff coverage is 16.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #271      +/-   ##
==========================================
- Coverage   40.51%   40.40%   -0.11%     
==========================================
  Files          30       30              
  Lines         980      980              
==========================================
- Hits          397      396       -1     
- Misses        583      584       +1     
Flag Coverage Δ
#unittests 40.40% <16.66%> (-0.11%) ⬇️
Impacted Files Coverage Δ
afew/filters/MeFilter.py 38.88% <16.66%> (-5.56%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2476428...c6b3da5. Read the comment docs.

@GuillaumeSeren
Copy link
Collaborator

Hello @ukleinek
First and foremost thank you for the interest and you submited patches on the project 👍

Note this is completely untested, so please look deeply before merging :-)

Yes actually there was some significant progress on the tests side of the project,
so creating a test for the MeFilter seems a fair preriquisite to merge this PR.

I will comment the diff.

@GuillaumeSeren
Copy link
Collaborator

Also ping @aidecoe original author of this filter

@aidecoe
Copy link
Contributor

aidecoe commented May 31, 2020

Hi ukleinek. What is the improvement? Or is it just a code refactor?

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