Code review comment for lp:~frankban/juju-quickstart/env-manage-base-models

Revision history for this message
Francesco Banconi (frankban) wrote :

Please take a look.

https://codereview.appspot.com/39380043/diff/1/quickstart/models/envs.py
File quickstart/models/envs.py (right):

https://codereview.appspot.com/39380043/diff/1/quickstart/models/envs.py#newcode69
quickstart/models/envs.py:69: a new, non preexisting environment.
On 2013/12/09 15:20:02, bac wrote:
> i think 'non preexisting' is redundant and should be removed.

Done.

https://codereview.appspot.com/39380043/diff/1/quickstart/models/envs.py#newcode207
quickstart/models/envs.py:207: os.fsync(temp_file.fileno())
On 2013/12/09 15:20:02, bac wrote:
> The flush and fscync are not handled by the close()?

Good question. According to
http://stackoverflow.com/questions/2333872/atomic-writing-to-file-with-python
it does not seem so.

https://codereview.appspot.com/39380043/diff/1/quickstart/models/envs.py#newcode245
quickstart/models/envs.py:245: # Why not using just env_data.copy()?
Because this way internal mutable
On 2013/12/09 15:20:02, bac wrote:
> Picky, but "Why not just use env_data.copy()?" reads better.

Done.

https://codereview.appspot.com/39380043/diff/1/quickstart/tests/test_utils.py
File quickstart/tests/test_utils.py (right):

https://codereview.appspot.com/39380043/diff/1/quickstart/tests/test_utils.py#newcode256
quickstart/tests/test_utils.py:256: '# in date 14/02/27 07:42:47.\n\n'
On 2013/12/09 15:20:02, bac wrote:
> s/in date/at/

> I think using the format "2014-02-27 07:42:47 UTC" would be better.

Done.

https://codereview.appspot.com/39380043/diff/1/quickstart/utils.py
File quickstart/utils.py (right):

https://codereview.appspot.com/39380043/diff/1/quickstart/utils.py#newcode151
quickstart/utils.py:151:
On 2013/12/09 15:20:02, bac wrote:
> perhaps now.isoformat(sep=' ')?

Done.

https://codereview.appspot.com/39380043/

« Back to merge proposal