Skip to content

Make the JsonFileLoader a context manager#137

Merged
minrk merged 1 commit intoipython:masterfrom
Carreau:context-manager
Feb 11, 2016
Merged

Make the JsonFileLoader a context manager#137
minrk merged 1 commit intoipython:masterfrom
Carreau:context-manager

Conversation

@Carreau
Copy link
Member

@Carreau Carreau commented Nov 26, 2015

On exit of context manager write file back to disk.
That makes it easy to update config.

On exit of context manager write file back to disk.
That makes it easy to update config.
@minrk minrk added this to the 4.2 milestone Nov 26, 2015
@minrk
Copy link
Member

minrk commented Nov 26, 2015

I like it! We can merge once we ship 4.1.

@Carreau
Copy link
Member Author

Carreau commented Dec 8, 2015

Todo, maybe default the config file to 0x600

@Carreau
Copy link
Member Author

Carreau commented Feb 8, 2016

Todo, maybe default the config file to 0x600

Ok, I don't know why I wrote that there, it's unrelated to this PR.

configuration to disk.
"""
with open(self.full_filename, 'w') as f:
f.write(json.dumps(self.config, indent=2))
Copy link
Member

Choose a reason for hiding this comment

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

If there is an error here, the file will be left empty due to the open(...'w'). Perhaps dump the config prior to opening the file, so that it raises before deleting existing contents.

Copy link
Member

Choose a reason for hiding this comment

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

...A failure mode you can test!

Copy link
Member Author

Choose a reason for hiding this comment

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

Did you take care of that after merging ? Or should I open an issue and do it (sorry I was unavailable to work on that yesterday)

minrk added a commit that referenced this pull request Feb 11, 2016
Make the JsonFileLoader a context manager
@minrk minrk merged commit 78e0fc5 into ipython:master Feb 11, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants