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.

Sign up for GitHub

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

Quote Engine Non-CDN Retrieval #48

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
+175 −4
Open

Quote Engine Non-CDN Retrieval #48

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1a159c7
WIP QE FIX
wazaundtechnik Oct 14, 2015
0d6dadf
Temporarily fix build. Not all aspect tested yet.
wazaundtechnik Oct 26, 2015
dde4de0
Fix lint
wazaundtechnik Oct 26, 2015
b5ac901
Test to cover all case. Finalize everything.
wazaundtechnik Oct 31, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Fix lint
@wazaundtechnik
wazaundtechnik committed Oct 26, 2015
commit dde4de033bd77b3da10091050c100d51cc34b002
20 changes: 10 additions & 10 deletions tests/test_quoteengine.py
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
from datetime import datetime
import json
from unittest.mock import patch

import pytest
@@ -10,7 +9,7 @@
'--- '
'quotes: '
' - q_no: 1'
' quote: "Kenapa kalo mau sahur tiba2 ngantuk? Karena Anda belum punya cinta yang menemani Anda sahur."'
' quote: "Kenapa kalo mau sahur tiba2 ngantuk? Karena Anda belum punya cinta yang menemani Anda sahur."' # noqa
' author: Anang Ferdi'
' author_bio: Dokter cinta veteran'
' tags: '
@@ -32,10 +31,12 @@
' - matematika '
)


@pytest.fixture
def qe():
Copy link

Choose a reason for hiding this comment

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

can you please use a more descriptive name? it's not obvious what qe stands for.

Copy link

Choose a reason for hiding this comment

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

before that, why did you put this as fixture? as I understand it, this is your system under test. shouldn't you explicitly instantiate QuoteEngine inside your test case and not provide this as fixture? this is not going to be used anywhere else (at least for now).

return QuoteEngine()


def test_refresh_time(qe):
with patch('tululbot.utils.TimeHelper') as mock_dt:
with patch('tululbot.utils.requests') as mock_requests:
@@ -49,9 +50,9 @@ def get_json(self):
response1.status_code = 200
response1.json = {
"commit": {
"sha": "beefbeef"
}
"sha": "beefbeef"
}
}

response1.ok = True
Copy link

Choose a reason for hiding this comment

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

I see. so you're assigning the properties here. can you please put them as instance variable and pass them to the constructor instead? that way it's easier to understand what a fake response has.

in general I'm not a fan of dynamically attaching properties like this. often times this leads to confusion like "where the hell this property comes from if it's not defined inside the class definition?"


@@ -63,16 +64,15 @@ def get_json(self):
qe._last_refresh = datetime(2000, 1, 6, 15, 8, 24)

result = qe.refresh_url_if_applicable()
assert result == True
assert result is True
Copy link

Choose a reason for hiding this comment

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

use assert result or assert not result. is is used for comparison with None.

result = qe.refresh_url_if_applicable()
assert result == False
assert result is False
result = qe.refresh_url_if_applicable()
assert result == False
assert result is False

qe._last_refresh = datetime(2000, 1, 6, 15, 8, 24)

result = qe.refresh_url_if_applicable()
assert result == True
assert result is True
result = qe.refresh_url_if_applicable()
assert result == False

assert result is False
6 changes: 2 additions & 4 deletions tululbot/utils.py
View file
Original file line number Diff line number Diff line change
@@ -41,7 +41,7 @@ def format_quote(self, q):
return '{q[quote]} - {q[author]}, {q[author_bio]}'.format(q=q)

def refresh_cache_if_applicable(self):
if (self.refresh_url_if_applicable() == False):
if (not self.refresh_url_if_applicable()):
return False
Copy link

Choose a reason for hiding this comment

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

why this method has to return boolean? I look at the usage and this return value is ignored by the caller anyway.

as a side note, my preference is to separate pure functions (no side-effect) and impure functions. pure functions should return a meaningful value to the caller while impure ones shouldn't. this is like procedures in Pascal. I find this separation makes the functions or methods easier to reason about.


body = requests.get(self._quote_url).text
@@ -62,7 +62,7 @@ def refresh_url_if_applicable(self):
res = requests.get(self._git_branch_check_url)

# Don't care broken request
if (res.ok == False):
if (not res.ok):
return False

json = res.get_json()
@@ -72,5 +72,3 @@ def refresh_url_if_applicable(self):
self._last_refresh = now

return True
Copy link

Choose a reason for hiding this comment

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

Hmm now this function has side-effect, but its return value is also needed. quite problematic. how about changing the flow to something like this:

def refresh_cache_if_applicable(self):
    if self.need_refresh():
        self.refresh_url()
        self.refresh_cache()

def need_refresh(self):
    return delta.seconds < self._refresh_interval

def refresh_url(self):
    # do refresh url here and don't return

def refresh_cache(self):
    # do refresh cache here and don't return

this way it's clear that need_refresh is pure while the others aren't.