Added storage capabilities to pgzero #99
Conversation
|
|
||
| .. method:: load() | ||
|
|
||
| Loads the data from disk. |
There was a problem hiding this comment.
This should cover what happens if there is no previous save data.
| @@ -102,6 +103,8 @@ def prepare_mod(mod): | |||
| set before the module globals are run. | |||
|
|
|||
| """ | |||
| fn_hash = hash(mod.__file__) % ((sys.maxsize + 1) * 2) | |||
There was a problem hiding this comment.
I think this hash might need to be stronger than this. Also I don't believe hash() is necessarily stable across Python interpreter versions. I'd suggest using hashlib.sha1().
|
|
||
| path = '' | ||
|
|
||
| """Behaves like a dictionary with a few extra functions. |
There was a problem hiding this comment.
This docstring needs to be the first thing in the class - above path = ''
|
|
||
| The name of the file will be the script's name hashed. | ||
|
|
||
| NOTE: If two scripts have the same name, they will load/save from/to |
There was a problem hiding this comment.
I would expect this only if they have exactly the same path in the filesystem, no?
There was a problem hiding this comment.
I was getting the name from mod.__file__ so basically, if people use the relative path for the script when calling pgzrun intro.py then I would only get intro.py.
I corrected this to check if the path is absolute and if it isn't I convert it to absolute, like so:
filename = mod.__file__
if not os.path.isabs(filename):
filename = os.path.join(os.getcwd(), filename)
fn_hash = hashlib.sha1(filename.encode('utf-8'))
Storage.set_app_hash(fn_hash.hexdigest())
| # If no file exists, it's fine | ||
| logger.debug('No file to load data from.') | ||
| except json.JSONDecodeError: | ||
| msg = 'Storage is corrupted. Couldn\'t load the data.' |
There was a problem hiding this comment.
You should use double-quotes for human readable strings so that you don't have to escape apostrophes :-)
| def _get_platform_pgzero_path(): | ||
| """Attempts to get the right directory in Windows/Linux.MacOS""" | ||
| if platform.system() == 'Windows': | ||
| return os.path.join(os.environ['APPDATA'], 'pgzero') |
There was a problem hiding this comment.
This will crash with KeyError if %APPDATA% is not defined. Does that happen? Is there a fallback?
There was a problem hiding this comment.
I'm not sure, I haven't used Windows in years. But I would assume it's always set like $HOME and $SHELL on Linux.
| """Attempts to get the right directory in Windows/Linux.MacOS""" | ||
| if platform.system() == 'Windows': | ||
| return os.path.join(os.environ['APPDATA'], 'pgzero') | ||
| return os.path.expanduser(os.path.join('~', '.config/pgzero/saves')) |
There was a problem hiding this comment.
This looks odd. The reason to use os.path.join() is so that you don't have to assume os.sep is /. But the second parameter contains /.
os.path.join() accepts multiple arguments, which will let you remove the /-s from .config/pgzero/saves.
Or you could assume that os.sep is / except on Windows, so just return `os.path.expanduser('~/.config/pgzero/saves').
There was a problem hiding this comment.
Indeed, rookie mistake.
| exists_mock.return_value = True | ||
| self.storage = Storage() | ||
|
|
||
| def test_get(self): |
There was a problem hiding this comment.
You're just testing the "happy path" here. You should have tests for the file not existing, or existing but containing invalid JSON.
| return result | ||
| elif isinstance(obj, dict): | ||
| for k, v in obj.items(): | ||
| result = cls._get_json_error_keys(v, '{}[\'{}\']'.format(json_path, k)) |
There was a problem hiding this comment.
You should check that k is a string: JSON only supports strings as dict keys.
Also on this line, you're better to write:
'{}[{!r}]'.format(json_path, k)as the repr of a string containing special characters is more complicated than "'{}'".format(s)
There was a problem hiding this comment.
This is done, need to write a unittest for it.
| storage.save() | ||
|
|
||
|
|
||
| Following on the previous example, when starting your program, you can load that data back in:: |
There was a problem hiding this comment.
Please could you add an example where we define defaults before we call storage.load()?
|
Is this feature included in v1.2? The FlappyBird example uses it but is not executable. |
|
This is actually merged, but I can't mark it as such. Closing. |
Added the code, unittests and documentation.
This PR attempts to add the functionality discussed on Issue #33