Refactor recording api methods and fix logic #613
Conversation
dbrenden
force-pushed
the
framework-logic-fixes
branch
2 times, most recently
from
ed2fbba
to
694219f
Compare
| } | ||
| with open(recording.storage_file, 'w') as fd: | ||
| fd.write(json.dumps(persistent)) | ||
| sys.exit(0) |
There was a problem hiding this comment.
Is this needed? I thought this function was already called when a SIGINT or whatever was received.
dbrenden
force-pushed
the
framework-logic-fixes
branch
from
dc739ee
to
281594d
Compare
|
Regarding your question on IRC about my try/except suggestion. Consider this try:
with open(your_file) as fd:
old_stuff = json.load(fd)
except IOError:
old_stuff = {}
next_id = 1
for key, value in old_stuff.iteritems():
do_more_things()If there is nothing from a previous run to load, then the loop will simply do nothing. This is more Pythonic than having branches and repeated logic for each case. We want to be as DRY as possible. |
| new_media = Multimedia(recording.config, recording.plugin_manager) | ||
| new_media.current_state = value['status'] | ||
| media_id = int(key) | ||
| #sets next_id to media_id if greater than current next_id |
There was a problem hiding this comment.
Not sure why flake8 isn't complaining about this. There should be a space after the # in a comment before the actual message.
I'm also not sure if it's even necessary as the logic is straightforward.
dbrenden
force-pushed
the
framework-logic-fixes
branch
from
3758873
to
c031112
Compare
| recording.media_dict[media_id] = { | ||
| 'media': new_media, | ||
| 'filename': filename, | ||
| 'filepath': filepath, |
There was a problem hiding this comment.
Does filepath have a default value somewhere if the state is not NULL?
dbrenden
force-pushed
the
framework-logic-fixes
branch
from
c031112
to
d39837d
Compare
| recording.media_dict = {} | ||
| for key, value in media_info.iteritems(): | ||
| new_media = Multimedia(recording.config, recording.plugin_manager) | ||
| if not value['null_flag']: |
There was a problem hiding this comment.
Maybe null_multimedia? It's also standard practice to start with the affirmative case first in an if/else.
| filepath2 = os.path.join(recording.config.videodir, 'mock_media_2') | ||
|
|
||
| recording.media_info['1'] = {'status': Multimedia.NULL, 'filename': 'mock_media_1', 'filepath': filepath1} | ||
| recording.media_info['2'] = {'status': Multimedia.NULL, 'filename': 'mock_media_2', 'filepath': filepath2} |
dbrenden
force-pushed
the
framework-logic-fixes
branch
from
b1800bb
to
091ee04
Compare
| recording.next_id = media_id + 1 | ||
|
|
||
| if new_media.current_state == Multimedia.NULL: | ||
| filename = value['filename'].split(".ogg")[0] |
There was a problem hiding this comment.
Single quotes please. The only exception is for function docstrings where double quotes are allowed.
dbrenden
force-pushed
the
framework-logic-fixes
branch
2 times, most recently
from
ca2bda5
to
07b444e
Compare
Refactored and fixed recording api logic. Implemented a shelve object so changes can persist more reliably and elminate the need for server teardown code. API tests now use pytest fixtures to create a baseline for tests instead of xunit style setup and teardown methods. Refactored and fixed logic of test_server.py tests.
dbrenden
force-pushed
the
framework-logic-fixes
branch
from
07b444e
to
7359f20
Compare
|
Thanks for your contribution! |
Refactored code of recording api setup, teardown, and
endpoint functions.
Added a get_current_state_str() method to Multimedia to make some code
in recording api cleaner.
Fix to recording api logic determining next_id.
Fix to test_server.py teardown to undo patching to media_dict
closes #629