If new msg text is None, fallback to empty str

scrapinghub / slackbot

A chat bot for Slack (https://slack.com).

MIT License

1.26k stars 396 forks source link

If new msg text is None, fallback to empty str #144

Closed tehranian closed 7 years ago

tehranian commented 7 years ago

Equivalent change to b6ba473eabb6f622d24cca3be19efb013dde8481, but happening in a different place. See description from #138.

jtatum commented 7 years ago

This seems to have come up a couple of times - if possible, can we add a unit or functional test for it? Let me know if you're comfortable doing that.

tehranian commented 7 years ago

Hi @jtatum ,

re: .get() and or '' being redundant - Yea, I thought that too when I looked at the code. As described more verbosely in #138, the value under the key text is None so .get() returns None, not the fallback of empty-string. Subsequently, passing None into the regexp matcher raises an exception.

Here's an illustrative example:

>>> d = {'text': None}
>>> repr(d.get('text', ''))
'None'
>>> repr(d.get('text', '') or '')
"''"

re: Adding tests - Sure, will do.

tehranian commented 7 years ago

Hi @jtatum,

Updated PR:

Added unit test for #138, already merged.
Moved logic of change for #144 to deeper-level, defending against bad input of text==None.
- Added unit test for #144.

Testing Done:

================================================= 40 passed, 1 skipped in 97.21 seconds ==================================================