-
Notifications
You must be signed in to change notification settings - Fork 26
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
base: master
Are you sure you want to change the base?
Conversation
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
There was a problem hiding this 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.
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]>
updated docstring for remove_words()
optimized and compressed remove_words() code
Fixed indentation
fixed docstring indentation
Co-Authored-By: RicoViking9000 <[email protected]>
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 |
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()
fixed assertion errors
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests are failing https://travis-ci.org/areebbeigh/profanityfilter/jobs/497660502
Anywhere defaults to True in the method signatures, but I will redo my testing. I appreciate your patience with me here. |
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
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? |
There was a problem hiding this 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
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]>
I will add code to the test file this weekend |
Added assertRaise testing
it's assert RAISES, not RAISE
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
This code has passed the tests |
2020 review: This should be added back into the main repo, since the current master has not been updated since December 2018. |
Hi @RicoViking9000 , Are you able to rebase this fork so we can have github actions against it. Regards, |
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)