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

Make extension paths relative to config file #112

Merged
merged 3 commits into from
Oct 31, 2023

Conversation

waylan
Copy link
Contributor

@waylan waylan commented Oct 30, 2023

There were a few different ways to accomplish this.

  1. Modify griffe to have it accept a base_path (which defaults to CWD) and resolve paths relative to it.
  2. Modify mkdocsstrings-python to resolve paths so that only absolute paths are passed to griffe.

Ultimately I went with the second option as griffe does not know about (and has no need to know about) mkdocstrings. It seems reasonable to me that griffe should expect to receive already resolved paths. As mkdocstrings-python is the glue between mkdocstrings and griffe, I thought that would be the most logical place to address the issue.

That said, much of the logic to separate the path from the rest of the data in the config options replicates the logic in griffe.extensions.load_extensions(). From a code efficiency point-of-view, it might have been the better to address this in griffe directly. However, I went from the logical API point-of-view. If the desire is to go the other way, let me know and I can do that.

Now, on to the actual fix itself...

The logical place to do this in mkdocstrings-python would be in config validation. However, there is no config validation for mkdocstring handlers at all. Building config validation would be a much bigger job than I want to tackle at this time. The next logical place to address this is where the extensions are actually accessed for the first time. Strangely, the handler class makes no mention of them anywhere (in attributes or the __init__ method). They are only referenced in the collect method which accepts them as an argument. I could have inserted the code inline within the collect method, but testing would have been difficult (mock objects with all sorts of hoops to trick the test). Therefore, I broke the functionality out into a separate method, which makes it easy to test. As an additional benefit, any future config validation can just call the existing method. Even if a general purpose config validation was added, we would still need this custom code. The extension config option does not just accept simple paths and so any validation code would need to be custom crafted for this option anyway. Well, now that custom code exists.

Finally, I will note that this code does not account for any of the extensions to be a Python object. It assumes each one is a string (either system path or Python dot notation). Of course, as we are getting the values from a config file, that is a reasonable assumption. However, it does mean that users cannot use YAML's special !!python function to pass in a Python object. If we want to allow that, then we need to import the various griffe extensions and type-check the values against them before processing values as strings. But given that users can already use both system paths and dot notation with a colon to specify a specific class, I see no need for that as well. I am simply pointing this out as it was possible before, but is no longer possible with the current fix in place.

I should also mention that paths which are already absolute are left unaltered. In other words, if a user is using mkdocs custom $config_dir variable to get absolute paths, that will continue to work. Although, with this fix that is no longer necessary. At least the user will not need to make changes to their config file upon update.

@waylan
Copy link
Contributor Author

waylan commented Oct 30, 2023

The failing test seems to be the same unrelated issue I was getting in mkdocstrings:

Material emoji logic has been officially moved into mkdocs-material
  version 9.4. Please use Material's 'material.extensions.emoji.twemoji'
  as mkdocs_material_extensions is deprecated and will no longer be
  supported moving forward. This is the last release.

@pawamoy
Copy link
Member

pawamoy commented Oct 31, 2023

Thank you for the PR and the clear explanation. I agree that duplicating logic from Griffe into the handler is not ideal, but it's not a lot of code, and is a good short/medium-term solution.

Some remarks:

  • paths to Python files without a .py extension are not supported, but it is reasonable to expect users to add a .py extension to their files
  • as you stated, it is also reasonable not to expect Python objects (loaded thanks to YAML's !!python tags) since users can pass the dotted path to modules/classes 👍

Quality failures are indeed caused by a deprecation message, I have fixed it in my project template, just need to update this project. Will do in another commit.

I allowed myself to push to your branch in order to:

  • fix the normalization condition to avoid false positives like package.pything (containing .py)
  • only normalize extensions config when needed
  • parametrize tests and add cases

It's looking good 🚀 Anything else before I merge?

@waylan
Copy link
Contributor Author

waylan commented Oct 31, 2023

Your changes look good to me. I don't use pytest regularly so I wasn't familiar with those features. Also, I hadn't considered testing for .py: (with the colon), which, as you note, avoids an undesirable match. All reasonable changes which improve the code.

As an aside, my original implementation was more complex as I was splitting on the colon to separate the class name from the path. Then I realized that the os.path functions don't care about that and it was unnecessary. Turns out we are not replicating all that much of griffe's logic.

In any event, thank you for getting these issues resolved so we can make full use of mkdocstrings without jumping through hoops.

@pawamoy
Copy link
Member

pawamoy commented Oct 31, 2023

Thanks a lot for your contributions!

@pawamoy pawamoy merged commit 5035e92 into mkdocstrings:main Oct 31, 2023
16 of 17 checks passed
@waylan waylan deleted the extpath branch October 31, 2023 19:36
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