-
Notifications
You must be signed in to change notification settings - Fork 439
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
Refactor conversion of None to empty introduced in #49 and #40 #93
Conversation
Current coverage is 95.18% (diff: 100%)@@ master #93 diff @@
==========================================
Files 34 34
Lines 2094 2098 +4
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
+ Hits 1993 1997 +4
Misses 101 101
Partials 0 0
|
Hi @jashgala, thanks for the PR! I hope to review it this week. |
@@ -184,3 +184,14 @@ def _safe_split(self, text): | |||
return words | |||
except: | |||
return text | |||
|
|||
def sanitize_if_none(self, text): | |||
"""Sanitizes text to ensure it is not None |
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.
Could you PEP-257-ify this docstring to the following?
Sanitize text to ensure it is not None.
"""Sanitizes text to ensure it is not None | ||
|
||
:type text:str | ||
:param text: String to sanitize |
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.
Also, a period here.
String to sanitize.
@jashgala sorry for the late review, thanks again for the PR. I've requested some minor tweaks to the docstring. Also, could you resolve the conflicts? Thanks! |
Continued in #100 (Changed target branch) |
Handled refactoring discussed in Issue #52.
Moved check for None to TextUtils and updated the github.py functions to use that instead of directly checking in the function itself.