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

Paths in config file are relative to the config file. #1376

Merged
merged 9 commits into from
Apr 4, 2018

Conversation

djeebus
Copy link
Contributor

@djeebus djeebus commented Jan 13, 2018

Ran into #543 today, saw that no one had started working on this, so decided to take a stab at it. I used pre_validation to coerce relative paths to absolute paths, which worked for my use case.

Two things I wasn't sure of where to go with:

  1. What happens if Config.load_file is passed something without a filename? I found a few tests where that was the case, but couldn't think of a use case otherwise. I reverted to the current behavior in this scenario (relative paths are left alone, so they're relative to the current working directory).
  2. A few of the tests relied on the current behavior (they processed config files in /tmp, and relied on the existence ./docs in order to pass validation. I cleaned up the tests, but used tempfile.TemporaryDirectory, which doesn't exist in py<3.2. Installing backports.tempfile took care of that, but made the imports a little more complex (you'll see them in the *_tests.py files).

Thanks for making this project available!

Fixes #543

@waylan
Copy link
Member

waylan commented Jan 13, 2018

Awesome! I'll have to take a closer look when I have some time.

@djeebus
Copy link
Contributor Author

djeebus commented Jan 13, 2018

Just found an issue: plugins aren't getting normalized properly. Should have it fixed soon too.

@djeebus
Copy link
Contributor Author

djeebus commented Jan 17, 2018

All ready now!

Copy link
Member

@waylan waylan left a comment

Choose a reason for hiding this comment

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

In addition to the specific comments below, we should also update the docs and add something to the release notes.

"""
The schema is a Python dict which maps the config name to a validator.
"""

self._schema = schema
self._schema_keys = set(dict(schema).keys())
self.fname = fname
Copy link
Member

Choose a reason for hiding this comment

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

I'm not crazy about naming this fname. Considering its intended use, we don't care about the "name" of the file, but rather the "path" to the file. Using fname indicates to me that I only need to pass in the name of the file, not the full path. However, in actual use, I need to pass in the absolute path (and it might even work if I left off the filename). Therefore, let's indicate that with the name used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe config_path?


config_dir = os.path.dirname(config.fname)
value = os.path.join(config_dir, value)
config[key_name] = value
Copy link
Member

@waylan waylan Jan 17, 2018

Choose a reason for hiding this comment

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

Rather than a pre-validation method, perhaps an abspath method would make more sense. Then anywhere we need an absolute path, just pass the value into the self.abspath method. And let's put it on the BaseConfigOption class so its available everywhere.

Note that you missed the custom_dir sub-setting of the theme setting here. That is not in a FilesystemObject subclass. And its entirely possible that Plugins could use various of the BaseConfigOption sub-classes or even create their own. to validate their own sub-settings. The point is, this ability to convert a relative path to an absolute path should be available from everywhere within the config validation classes.

Copy link
Member

Choose a reason for hiding this comment

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

And I just realized the problem with this. The config (which contains the base path) is only available from the pre_validation and post_validation methods. Therefore, we can only do this from there.

@waylan
Copy link
Member

waylan commented Apr 3, 2018

Hmm, for some reason the AppVeyor py33 tests are running on Python 2.7.

@waylan
Copy link
Member

waylan commented Apr 3, 2018

The py33 tests are being handled in #1453. Guess we can move on with this issue by updating the docs/release notes.

@waylan waylan mentioned this pull request Apr 4, 2018
Guess I broke this when I rebased.
@waylan waylan added this to the 1.0.0 milestone Apr 4, 2018
@waylan waylan merged commit a3e60f1 into mkdocs:master Apr 4, 2018
waylan added a commit to waylan/mkdocs that referenced this pull request Apr 4, 2018
Fixes mkdocs#1291 and made possable by the work in mkdocs#1376.
waylan added a commit that referenced this pull request Apr 4, 2018
Fixes #1291 and made possable by the work in #1376.
@djeebus djeebus deleted the relative-paths branch April 26, 2018 00:38

Previously any relative paths in the various configuration options were
resolved relative to the current working directory. They are now resolved
relative to the configuration file. As the documentation has always encouraged

Choose a reason for hiding this comment

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

@waylan I couldn't find it anywhere in the docs, but what about for inherited config files? Are the paths in those inherited config files relative to the parent config file or the child config file?

Copy link
Contributor

@oprypin oprypin Jun 20, 2023

Choose a reason for hiding this comment

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

Please don't necro-bump and especially don't tag people while doing so.

The answer is here

As a reminder, all path based configuration options must be relative to the
primary configuration file and MkDocs does not alter the paths when merging.

Copy link
Contributor

Choose a reason for hiding this comment

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

(you can ask a question as a Discussion instead)

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

4 participants