Make extension paths relative to config file#112
Conversation
|
The failing test seems to be the same unrelated issue I was getting in |
|
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:
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:
It's looking good 🚀 Anything else before I merge? |
|
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 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 In any event, thank you for getting these issues resolved so we can make full use of mkdocstrings without jumping through hoops. |
|
Thanks a lot for your contributions! |
PR #112: mkdocstrings/python#112 Co-authored-by: Timothée Mazzucotelli <pawamoy@pm.me>
There were a few different ways to accomplish this.
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 thecollectmethod which accepts them as an argument. I could have inserted the code inline within thecollectmethod, 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
!!pythonfunction 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_dirvariable 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.