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

Fixes #283. If the mkdocs.yml is completely empty there is a traceback #288

Merged
merged 6 commits into from Dec 17, 2014

Conversation

@kaiix
Copy link
Contributor

@kaiix kaiix commented Dec 16, 2014

No description provided.

user_config.update(options)
local_config = yaml.load(fp)
if local_config:
user_config.update(local_config)

This comment has been minimized.

@d0ugal

d0ugal Dec 16, 2014
Member

I think this should be local_config.update(user_config) with your renaming which I don't fully understand it has reversed it.

@d0ugal
Copy link
Member

@d0ugal d0ugal commented Dec 16, 2014

Looks like a decent start. I'd love to see some tests for this and it would be good idea to handle other related problems at the same time. The YAML may not be empty, but could contain a totally invalid data structure.

It might be best to do something like:

with open(filename, 'r') as fp:
    user_config = yaml.load(fp)
    if not isinstance(user_config, dict):
        raise ConfigurationError("The mkdocs.yml file is invalid. See http://www.mkdocs.org/user-guide/configuration/ for more information.")
@kaiix
Copy link
Contributor Author

@kaiix kaiix commented Dec 17, 2014

I can't get a better name than user_config here, and to be consistent with following code, I just keep using it.

options = options or {}
if 'config' in options:
filename = options['config']
user_config = options or {}

This comment has been minimized.

@d0ugal

d0ugal Dec 17, 2014
Member

This is the naming that confused me. Why change this from options to user_config? options are the command line options and user_config is the configuration loaded from the users YAML.

This comment has been minimized.

@kaiix

kaiix Dec 17, 2014
Author Contributor

In another way, you can think user_config is constructed from either options from command line or from a user defined yaml file.

This comment has been minimized.

@d0ugal

d0ugal Dec 17, 2014
Member

Ok, I don't think that change helps. It would be better to change the names back as this is still introducing a bug...

yaml_config = yaml.load(fp)
if not isinstance(yaml_config, dict):
raise ConfigurationError("The mkdocs.yml file is invalid. See http://www.mkdocs.org/user-guide/configuration/ for more information.")
user_config.update(yaml_config)

This comment has been minimized.

@d0ugal

d0ugal Dec 17, 2014
Member

The options should take precedent over the YAML config. So you can overwrite settings with a command line flag. This line does the opposite.

This comment has been minimized.

@kaiix

kaiix Dec 17, 2014
Author Contributor

Ok, I understand.

@d0ugal d0ugal added this to the 0.12.0 milestone Dec 17, 2014
@d0ugal
Copy link
Member

@d0ugal d0ugal commented Dec 17, 2014

Thanks!

@d0ugal d0ugal closed this Dec 17, 2014
@d0ugal d0ugal reopened this Dec 17, 2014
d0ugal added a commit that referenced this pull request Dec 17, 2014
Fixes #283. If the mkdocs.yml is completely empty there is a traceback
@d0ugal d0ugal merged commit 1eaac86 into mkdocs:master Dec 17, 2014
1 check passed
1 check passed
@tomchristie
continuous-integration/travis-ci The Travis CI build passed
Details
@d0ugal
Copy link
Member

@d0ugal d0ugal commented Dec 17, 2014

(Sorry, I intended to merge, not close 😄)

@kaiix kaiix deleted the kaiix:empty-config branch Dec 18, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants