-
Notifications
You must be signed in to change notification settings - Fork 54
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
Ensure paths mentioned in Sphinx docs are valid #1183
Changes from 5 commits
80c3bf4
b952e2b
579913a
b4e5b96
819f1b7
ca42475
892cbf3
42ff1fc
bda6df5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,84 @@ | ||
import functools | ||
import logging | ||
import os | ||
import subprocess | ||
from pathlib import Path | ||
from docutils import nodes | ||
|
||
from sphinx.application import Sphinx as SphinxApp | ||
from sphinx.roles import EmphasizedLiteral | ||
|
||
|
||
_logger = logging.getLogger(f"sphinx.ext.{__name__}") | ||
|
||
|
||
@functools.lru_cache | ||
def _get_repo_root() -> Path: | ||
try: | ||
repo_root = subprocess.check_output( | ||
["git", "rev-parse", "--show-toplevel"], | ||
text=True, | ||
).strip() | ||
except subprocess.CalledProcessError as e: | ||
raise RuntimeError("Unable to get repository root") from e | ||
return Path(repo_root).resolve() | ||
|
||
|
||
class RepoPath(EmphasizedLiteral): | ||
@property | ||
def path(self) -> Path: | ||
repo_root = _get_repo_root() | ||
return repo_root / self.text | ||
|
||
def run(self): | ||
if not self.path.exists(): | ||
_logger.warning("path %r does not exist", self.text) | ||
return super().run() | ||
|
||
|
||
class PathRoleBase: | ||
def __init__(self, base_path=""): | ||
self.base_path = Path(base_path) | ||
|
||
def validate(self, path: Path) -> None: | ||
if not path.exists(): | ||
relpath = path.relative_to(self.base_path) | ||
_logger.warning( | ||
"path %r does not exist within %r", str(relpath), str(self.base_path) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. for consistency maybe use os.fspath() rather than str() for both of these There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point, I think I was just being lazy 😅 |
||
) | ||
|
||
def display(self, path: Path, text="") -> str: | ||
raise NotImplementedError | ||
|
||
def __call__( | ||
self, | ||
name: str, | ||
rawtext: str, | ||
text: str, | ||
lineno: int, | ||
options: dict = None, | ||
content: list = None, | ||
): | ||
|
||
path = self.base_path / text | ||
self.validate(path) | ||
path_to_display = self.display(path=path) | ||
node = nodes.literal(path_to_display, path_to_display) | ||
return [node], [] | ||
|
||
|
||
class Relpath(PathRoleBase): | ||
def display(self, path: Path, text="") -> str: | ||
return os.fspath(path.relative_to(self.base_path)) | ||
|
||
|
||
class Filename(PathRoleBase): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe override validate to add a is_file() check as well for this type There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good idea, I'll add that. |
||
def display(self, path: Path, text="") -> str: | ||
return path.name | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. consider returning something like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good suggestion, maybe to keep in mind for a future iteration? I see the pattern you're referring to, but having the role handle more complicated formatting (since the prose would have to be formatted differently than the actual path) looked quite a bit trickier than the current implementation. |
||
|
||
|
||
def setup(app: SphinxApp): | ||
repo_root = _get_repo_root() | ||
|
||
app.add_role("path", Relpath(repo_root)) | ||
app.add_role("filename", Filename(repo_root)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why the Path constructor here? all the usage already passes a Path into the PathRoleBase
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly for robustness/defensiveness: the internals only work if
self.base_path
is apathlib.Path
, andpathlib.Path
instances are immutable so creating a newPath
is easier than havingisinstance(..., Path)
checks.