Ensure paths mentioned in Sphinx docs are valid #1183
Conversation
lbianchi-lbl
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1183 +/- ##
=======================================
Coverage 38.52% 38.52%
=======================================
Files 163 163
Lines 36852 36852
Branches 5994 5994
=======================================
Hits 14199 14199
Misses 21544 21544
Partials 1109 1109 ☔ View full report in Codecov by Sentry. |
|
|
||
| class PathRoleBase: | ||
| def __init__(self, base_path=""): | ||
| self.base_path = Path(base_path) |
There was a problem hiding this comment.
why the Path constructor here? all the usage already passes a Path into the PathRoleBase
There was a problem hiding this comment.
Mostly for robustness/defensiveness: the internals only work if self.base_path is a pathlib.Path, and pathlib.Path instances are immutable so creating a new Path is easier than having isinstance(..., Path) checks.
docs/source/_repopath.py
Outdated
| 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.
for consistency maybe use os.fspath() rather than str() for both of these
There was a problem hiding this comment.
Good point, I think I was just being lazy 😅
| return os.fspath(path.relative_to(self.base_path)) | ||
|
|
||
|
|
||
| class Filename(PathRoleBase): |
There was a problem hiding this comment.
maybe override validate to add a is_file() check as well for this type
There was a problem hiding this comment.
Good idea, I'll add that.
|
|
||
| class Filename(PathRoleBase): | ||
| def display(self, path: Path, text="") -> str: | ||
| return path.name |
There was a problem hiding this comment.
consider returning something like path.name + ", and this file is located in " + os.fspath(path.parent.relative_to(self.base_path)) since that pattern occurs repeatedly in the usage
There was a problem hiding this comment.
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.
Resolves: #1180
Changes proposed in this PR:
:path:and:filename:roles to display paths relative to repository root with validation (a warning is emitted if the referred path is not found during the build):path:/:filename:rolesThis is how it looks when non-existing paths are used:
Legal Acknowledgement
By contributing to this software project, I agree to the following terms and conditions for my contribution: