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.

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

use json format for config file #998

Merged
merged 1 commit into from
Jul 15, 2019
Merged

Conversation

dt-rush
Copy link
Contributor

@dt-rush dt-rush commented Jul 15, 2019

This PR replaces the config file format with human-friendly JSON, and also adds code to seamlessly convert old configs to JSON if detected.


rationale

As the recent API key overload made clear (issue #551), users being able to edit the config themselves is an important feature (modifying the configured default API key is not really feasible by hand, as one user remarked).

The config is currently stored as a pickled binary file, which is not user-friendly.

Pickled binary storage is not necessary for any reasonable disk-space reasons. The difference in the old config style vs json is on the order of tens of bytes, as can be seen:

pi@raspberrypi:~/.config/mps-youtube $ python3

>>> import pickle
>>> import json
>>> with open('config', 'rb') as cf:
...     with open('config.json', 'w') as cfj:
...             json.dump(pickle.load(cf), cfj)
... 
>>> 
KeyboardInterrupt

pi@raspberrypi:~/.config/mps-youtube $ ls -l
total 20
-rw-r--r-- 1 pi pi  708 Jul 15 03:14 config
-rw-r--r-- 1 pi pi  676 Jul 15 03:47 config.json

Signed-off-by: dt-rush nickp@balena.io

Signed-off-by: dt-rush <nickp@balena.io>
@ids1024 ids1024 merged commit 8ee685a into mps-youtube:develop Jul 15, 2019
@ids1024
Copy link
Contributor

ids1024 commented Jul 15, 2019

Thanks! The set command exists to modify configuration, and I don't see a strongly compelling reason for the format to be text based. But generally text based configuration formats are nicer to have, and there isn't much reason not to.

@TsukiZombina
Copy link

Thanks! The set command exists to modify configuration, and I don't see a strongly compelling reason for the format to be text based. But generally text based configuration formats are nicer to have, and there isn't much reason not to.

The set command doesn't work for me, just changing configuration file, as I mention in #551.

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.

3 participants