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 TOML for the configuration file #59

Closed
wants to merge 4 commits into from
Closed

Use TOML for the configuration file #59

wants to merge 4 commits into from

Conversation

WhyNotHugo
Copy link
Member

@WhyNotHugo WhyNotHugo commented Nov 24, 2016

Use TOML as a config format:

  • TOML is clearer than our current config format, unambiguous, and other pros I believe we already know.
  • It may be a bit premature though, since the current format hasn't given any problems.
  • TOML is becoming a lot more popular, especially in the python community.

Set default values when we load the config, rather than scatter them around the app, which should make everything a bit cleaner.

Closes #31

@WhyNotHugo
Copy link
Member Author

@untitaker I'd love some feedback on whether this is worthwhile. Not sure it's of any actual use, though it makes sense for have todoman and vdirsyncer use the same format.

@untitaker
Copy link
Member

I am really not sure whether adding a new dependency is worth it if you currently have two config keys, both of which are strings.

Switch to TOML won't happen that soon for vdirsyncer. The intermediate goal in vdirsyncer's case is to get everybody to switch to quoted strings in their config.

return config
with open(path) as conffile:
config = toml.loads(conffile.read())
# TODO: Validate required fields here (ie: path)
Copy link
Member

Choose a reason for hiding this comment

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

If you're adding dependencies already you might as well use JSON-Schema for this

Copy link
Member Author

Choose a reason for hiding this comment

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

Huh, yeah, I'll look at validating the config file, since that would be a stronger bonus. But I don't see how to do this without another dependency (python-jsonschema).

@WhyNotHugo
Copy link
Member Author

Added schema validation, and some tests. I believe we can also clean up a few other bits of code, since we can always assume that all configuration variables exist, and are valid.

@untitaker
Copy link
Member

Implementation looks good to me. This adds 0.1s overhead on my machine though.

@untitaker
Copy link
Member

Oh, I see that you're using toml, I'd rather recommend pytoml. Its code quality is better.

@WhyNotHugo
Copy link
Member Author

I'd rather recommend pytoml

I'll take a look at it. The interface seems pretty much the same.

@untitaker
Copy link
Member

Yeah the interface is always json-ish. But toml.loads is 230 lines.

@WhyNotHugo
Copy link
Member Author

WhyNotHugo commented Jan 31, 2017

I've opted for configobj (#31) rather than toml.

  • ini works well enough for us.
  • Making users change their config file is a downside.
  • Errors for pytoml are rather unfriendly (See Tab in string yielding error avakar/pytoml#5)
  • The validation schema syntax for JSON (which is what's used here) is horrible.
  • One extra dependency vs two extra dependencies.

@WhyNotHugo WhyNotHugo closed this Jan 31, 2017
@WhyNotHugo WhyNotHugo deleted the toml-conf branch January 31, 2017 18:11
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.

None yet

2 participants