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

Improvements to remove_word + new method: remove_word_list #18

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

Conversation

RicoViking9000
Copy link

When I was using your (wonderful by the way) profanity filter module, I did happen to realize that there was a limitation for my use - there was no way to remove words from the extra_censor_list except for using the restore_words() method and re-defining the extra_censor_list without the word to remove.

I have forked and modified the remove_word() method to better suit my needs and possibly the needs of other people. If you see this as something other people might make use of, you're welcome to merge.

In summary, I added an argument to remove_word (defaults to True), which, if True, checks if the word is in extra_censor_list before moving on to the other lists. In addition, previously there was no easy way to remove a word from the custom_censor_list without redefining, so I modified the method to, by default, do that if a custom_censor_list is used.

I also added a new method - remove_word_list, which makes sense to me as define_words takes a list, so now we have a method that takes a list and essentially does the same thing as remove_word(), but with a list.

Finally, I updated the Docstring slightly to be in conjunction with my edits.

The most efficient and working changes are the latest commit (60b1ad5)

New features
     * The remove_word() method now can remove words from an instance's custom_censor_list if it's used
     * New argument - Universal, default True: if True, remove word from wherever it is; namely extra_censor_list if applicable
     * New method - remove_word_list() - remove a list of words, following similar format as remove_word()
Updated remove_word():
     *If _custom_censor_list is used, remove words from there instead of returning error for value not in _censor_list
     *New arg - anywhere: If true, remove words from _extra_censor_list if word is there (checks there first)
     *Updated docstring as appropriate
New method remove_word_list():
    *Similar to remove_word(), but pass a list instead of string
*If _custom_censor_list is used, remove words from there instead of returning error for value not in _censor_list
     *New arg - anywhere: If true, remove words from _extra_censor_list if word is there (checks there first)
     *Updated docstring as appropriate
New method remove_word_list():
    *Similar to remove_word(), but pass a list instead of string
Copy link
Owner

@areebbeigh areebbeigh left a comment

Choose a reason for hiding this comment

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

The improvement ideas are good. A few changes and this PR should be good to merge.

profanityfilter/profanityfilter.py Outdated Show resolved Hide resolved
profanityfilter/profanityfilter.py Outdated Show resolved Hide resolved
profanityfilter/profanityfilter.py Outdated Show resolved Hide resolved
profanityfilter/profanityfilter.py Outdated Show resolved Hide resolved
profanityfilter/profanityfilter.py Outdated Show resolved Hide resolved
profanityfilter/profanityfilter.py Outdated Show resolved Hide resolved
profanityfilter/profanityfilter.py Outdated Show resolved Hide resolved
profanityfilter/profanityfilter.py Outdated Show resolved Hide resolved
profanityfilter/profanityfilter.py Outdated Show resolved Hide resolved
profanityfilter/profanityfilter.py Outdated Show resolved Hide resolved
profanityfilter/profanityfilter.py Outdated Show resolved Hide resolved
profanityfilter/profanityfilter.py Outdated Show resolved Hide resolved
profanityfilter/profanityfilter.py Outdated Show resolved Hide resolved
@areebbeigh
Copy link
Owner

Okay, now this needs tests.

If you're not familiar with writing tests take a look at how https://github.com/areebbeigh/profanityfilter/blob/master/tests/test_profanity.py is written. That's the file you'll be working with. Also, take a look at nose on PyPI. 👍

@RicoViking9000
Copy link
Author

I will get to that this weekend, I appreciate the information

Updated remove_word() for new features - removing from _extra_censor_list
Added remove_words()
@RicoViking9000
Copy link
Author

I also have code locally (off github) for get_word_boundaries() and set_word_boundaries(bool), I can add them here if you prefer; I'm using these methods for my application, but you're welcome to keep it locked to instantiation only, totally up to you

Copy link
Owner

@areebbeigh areebbeigh left a comment

Choose a reason for hiding this comment

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

tests/test_profanity.py Outdated Show resolved Hide resolved
@RicoViking9000
Copy link
Author

Anywhere defaults to True in the method signatures, but I will redo my testing. I appreciate your patience with me here.

@areebbeigh
Copy link
Owner

In that case, test if anywhere=True does remove from both the lists and False doesn't. :)

program now properly tests using both values of the 'anywhere' bool for remove_word() and remove_words()
fixed misspelling of 'chocolate'
I think I finally realize how the testing works here
@RicoViking9000
Copy link
Author

Should I remove the lines that cause ValueErrors due to a certain word not being in the profanity list for remove_word()/remove_words(), and replace them with lines that pass the TravisCi testing?

Copy link
Owner

@areebbeigh areebbeigh left a comment

Choose a reason for hiding this comment

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

When the word to be removed isn't found, we want an error to be raised. This behavior is expected and can be tested with https://docs.python.org/3/library/unittest.html#unittest.TestCase.assertRaises

tests/test_profanity.py Outdated Show resolved Hide resolved
tests/test_profanity.py Outdated Show resolved Hide resolved
tests/test_profanity.py Outdated Show resolved Hide resolved
tests/test_profanity.py Outdated Show resolved Hide resolved
tests/test_profanity.py Outdated Show resolved Hide resolved
tests/test_profanity.py Outdated Show resolved Hide resolved
tests/test_profanity.py Outdated Show resolved Hide resolved
tests/test_profanity.py Outdated Show resolved Hide resolved
tests/test_profanity.py Outdated Show resolved Hide resolved
areebbeigh and others added 9 commits March 7, 2019 21:16
Co-Authored-By: RicoViking9000 <[email protected]>
Co-Authored-By: RicoViking9000 <[email protected]>
Co-Authored-By: RicoViking9000 <[email protected]>
Co-Authored-By: RicoViking9000 <[email protected]>
Co-Authored-By: RicoViking9000 <[email protected]>
Co-Authored-By: RicoViking9000 <[email protected]>
Co-Authored-By: RicoViking9000 <[email protected]>
Co-Authored-By: RicoViking9000 <[email protected]>
Co-Authored-By: RicoViking9000 <[email protected]>
@RicoViking9000
Copy link
Author

I will add code to the test file this weekend

fixed improper uses of "raises" testing
Following programming logic and the way the remove words method was coded, the program would remove words individually, and would stop at the point it encountered the error, not before. Fixed incorrect assertion test due to this
@RicoViking9000
Copy link
Author

This code has passed the tests

@DonaldTsang
Copy link

2020 review: This should be added back into the main repo, since the current master has not been updated since December 2018.

@duttonw
Copy link
Collaborator

duttonw commented Nov 25, 2024

Hi @RicoViking9000 ,

Are you able to rebase this fork so we can have github actions against it.

Regards,

@duttonw

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.

4 participants