Skip to content

Conversation

@shyamd
Copy link

@shyamd shyamd commented Apr 23, 2020

This fixes an issue finding the relative path when the root package is a namespace package. This is likely to break if the namespace package is nested in the module like my_package.plugins and you want to document something that lives in that plugins namespace.

Copy link
Member

@pawamoy pawamoy left a comment

Choose a reason for hiding this comment

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

I added a few questions just to make sure I understand correctly. Could add your answers as comments in the code too, to the attention of future readers (you and I included 😄)?

So, if I get it right, it's doing the same thing as before, except better, because it not only tries the top package name, but all the possible parts concatenation down to the before-last part? And you also try to import the module if it's not already loaded in sys.modules.

If yes, then it looks good. Do the tests pass? Currently GitLab does not support triggering CI for PRs from forks, sorry about that... I might need to setup GitHub Actions or Azure devops or whatever this is instead 😕

In the related issue, you said that testing for native namespace packages would be a giant pain. Could you explain why? Is it because the tests folder has an __init__.py, which means to test native namespace package we would need to put it outside of this folder?

Finally, could you explain how this could break? You mention a nested module but I'm not sure to understand 😅 Maybe a folder tree example would help!

@shyamd
Copy link
Author

shyamd commented Apr 23, 2020

The nested try/except always bothers me so I thought something like this might be a bit more readable:

        for namespace in namespaces:
            try:
                importlib.import_module(namespace)
                top_package = sys.modules[namespace]
            except (KeyError, ImportError):
                """
                Triggered if we can't import the package via importlib
                Or if package isn't findable by it's canonical name even after importing
                """
                return ""

            try:
                top_package_path = Path(inspect.getabsfile(top_package)).parent
                return str(Path(self.file_path).relative_to(top_package_path.parent))
            except ValueError:
                return ""
            except TypeError:
                pass

@pawamoy
Copy link
Member

pawamoy commented Apr 23, 2020

This is definitely more readable! We always enforce the module is imported, and I guess it's not an expensive operation when it's already loaded, so it's fine.

@pawamoy
Copy link
Member

pawamoy commented Apr 23, 2020

Alright, this looks great. Anything more you'd like to add? Also, do you mind if I squash the commits? I'm using the angular style, to ease the generation of the changelog.

EDIT: oh, I forgot about the tests! Do you think you could write some?

@shyamd
Copy link
Author

shyamd commented Apr 23, 2020

I'm trying to figure out how to build a test fixture since namespace packages have to be installed to get them to be located properly.

@pawamoy
Copy link
Member

pawamoy commented Apr 25, 2020

Nice! The TypeError is not covered but I think I can add a test for it myself to get used to namespace packages. Ready to merge?

@shyamd
Copy link
Author

shyamd commented Apr 25, 2020

Hmmm. Actually no, that should have been covered since i'm attempting to load a namespace directory. Let me play with test a bit more.

@shyamd
Copy link
Author

shyamd commented Apr 25, 2020

Ok. That was just a bad test. I've updated it to actually check the relative_file_path and ensure it points to the __init__.py file of the subspace module. This is both a good test case and an edge case since the previous code was failing when referencing the top subspace module.

I think this is ready to merge now.

@shyamd
Copy link
Author

shyamd commented Apr 25, 2020

I can setup pushing to codecov if you setup the account and add the codecov token to github secrets for this repo. I just need to know the name of the secret to reference it in the workflow.

@pawamoy
Copy link
Member

pawamoy commented Apr 25, 2020

Not a fan of codecov 😕 (generally not a fan of third-party web services)

If we can get a badge from GitHub workflow to show coverage percent in README, it's good enough for me! If not, well... it's fine. Not really important.

Anyway, thank you very much @shyamd! I really appreciate your work on this.

@pawamoy pawamoy merged commit e99459f into mkdocstrings:master Apr 25, 2020
@shyamd shyamd deleted the shyamd branch April 25, 2020 18:28
pawamoy pushed a commit that referenced this pull request Apr 25, 2020
Native namespace packages don't have an `__init__.py` module.
This commit adds support for finding relative file path
of such packages.

References: #19, #22.
@pawamoy
Copy link
Member

pawamoy commented Apr 25, 2020

Thanks again! 🎉

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