Skip to content

Avoid infinite loop in module_name_from_path #89

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.

Sign up for GitHub

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

Merged
lagru merged 7 commits into from
Oct 23, 2025
+92 −17
Merged

Avoid infinite loop in module_name_from_path #89

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
40c1f49
Exit loop
geographika Oct 9, 2025
fdb3fd5
Stop at root
geographika Oct 10, 2025
b33fbb8
Avoid endless loop for relative paths in `module_name_from_path`
lagru Oct 18, 2025
ac8c34b
Warn in case docstub is run on a subpackage only
lagru Oct 18, 2025
e16ba37
Appease mypy pointing out call to untyped function in tests
lagru Oct 18, 2025
05c6ac9
Appease mypy pointing out a missing return type annotation
lagru Oct 18, 2025
2d8f632
Merge branch 'main' into ghpr/89_loop-fix
lagru Oct 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/docstub/_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ def module_name_from_path(path):
if is_in_package:
name_parts.insert(0, directory.name)
directory = directory.parent
break
Copy link
Copy Markdown
Member

@lagru lagru Oct 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Given the failing test, this seems like it may not be the correct fix.

Could you perhaps provide more context on how the original bug occurred? Maybe we can construct a minimal reproducing example for it?

Copy link
Copy Markdown
Member

@lagru lagru Oct 10, 2025

Choose a reason for hiding this comment

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

At least in the context of the test case, it seems to deal fine with the presence of a __init__.py.

Copy link
Copy Markdown
Member

@lagru lagru Oct 10, 2025

Choose a reason for hiding this comment

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

In OSGeo/gdal#13198, in what directory did you run docstub? It's meant to be passed the path to a package, including the root directory of that package.

Copy link
Copy Markdown
Contributor Author

@geographika geographika Oct 10, 2025

Choose a reason for hiding this comment

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

Ah - that explains it. I'm running docstub on Python files generated from SWIG from within the same folder, along with an __init__.py file. I've updated the PR to add a check to avoid hanging if run in the same folder, but it works fine pointing to the package folder too with: docstub run ./osgeo --no-cache --config C:\docs\gdal\swig\python\pyproject.toml.

Copy link
Copy Markdown

@dbaston dbaston Oct 10, 2025

Choose a reason for hiding this comment

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

For me it works if an absolute path is provided to docstub, but if I run it from the module directory itself (with docstub run . then we get an infinite loop because the parent of . is ., apparently.

Copy link
Copy Markdown
Member

@lagru lagru Oct 10, 2025

Choose a reason for hiding this comment

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

That fits. I tried to make docstub preserve relative paths because that makes output and (error) messages a bit more readable. But absolute paths are the more robust option.

I definitely think docstub should handle this case more gracefully: docstub run . where . is inside a package.

Is there some particular behavior you would expect in that case? I'm thinking about that myself right now.

Copy link
Copy Markdown

@dbaston dbaston Oct 10, 2025

Choose a reason for hiding this comment

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

You could convert the relative path to an absolute path with path = path.resolve() near the top of the function?

Copy link
Copy Markdown
Member

@lagru lagru Oct 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Oh, I'm more asking if it makes sense that docstub supports running inside or only on part of a Python package at all. Which types are matched depends on what types are collected throughout the package.

Right now I'm thinking, support this use case but warn that running on partial packages may lead to incomplete results.

Copy link
Copy Markdown
Contributor Author

@geographika geographika Oct 11, 2025

Choose a reason for hiding this comment

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

Oh, I'm more asking if it makes sense that docstub supports running inside or only on part of a Python package at all. Which types are matched depends on what types are collected throughout the package.

Right now I'm thinking, support this use case but warn that running on partial packages may lead to incomplete results.

For info, I get the same output if I run within the folder (using . which works with the latest commit), or outside with a full or relative path (./osgeo). All .py files are in the same folder though, so maybe this is more relevant to projects with .py files in different subfolders?

Both approaches require some types to be added to the pyproject.toml (I'm not sure why as the same types in some docstrings don't throw errors): https://github.com/OSGeo/gdal/blob/b8d0f72f306e7fc0b5a511d96221277797301be5/swig/python/pyproject.toml#L47

Copy link
Copy Markdown
Member

@lagru lagru Oct 18, 2025

Choose a reason for hiding this comment

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

I don't really understand how the osgeo package is structured or populated. I assume something like the osgeo.osr module is created during build time? So I'm not sure if docstub is missing something or this is simply a setup that docstub can't really support.

else:
break

Expand Down
Loading