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

Refactor conversion of None to empty introduced in #49 and #40 #93

Closed
wants to merge 1 commit into from
Closed

Refactor conversion of None to empty introduced in #49 and #40 #93

wants to merge 1 commit into from

Conversation

jashgala
Copy link
Contributor

@jashgala jashgala commented Nov 6, 2016

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.

@codecov-io
Copy link

codecov-io commented Nov 6, 2016

Current coverage is 95.18% (diff: 100%)

Merging #93 into master will increase coverage by <.01%

@@             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          

Powered by Codecov. Last update 5a24a9b...845cef8

@donnemartin
Copy link
Owner

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
Copy link
Owner

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
Copy link
Owner

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.

@donnemartin
Copy link
Owner

@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!

@jashgala
Copy link
Contributor Author

jashgala commented Dec 6, 2016

Continued in #100

(Changed target branch)

@jashgala jashgala closed this Dec 6, 2016
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.

3 participants