|WARNING| devtools: add script to find orphan documentation files

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: WARNING
https://dpdk.org/patch/167037

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-07-30

# DPDK Patch Review: devtools/check-doc-orphans.py

## Summary

This patch adds a Python script to detect orphaned documentation files in the DPDK tree. The script builds a reference graph from Sphinx directives and reports files that are no longer referenced. Overall, the implementation is sound with good correctness practices. A few minor style and documentation improvements are suggested below.

---

## Findings

### **Warnings**

#### 1. Missing module-level docstring detail
**File:** `devtools/check-doc-orphans.py:11-12`

The module docstring could be more specific about usage and integration points.

**Current:**
```python
"""
Report orphaned documentation source and image files.

Removing an example or a library leaves its images behind. Walk the doc
tree, build a reference graph from the Sphinx directives, roles and
toctrees, then list every text and image file that no longer has a path
from a documentation root.
"""
```

**Suggested:** Add a usage example or mention that this is intended for CI/developer use:
```python
"""
Report orphaned documentation source and image files.

Removing an example or a library leaves its images behind. Walk the doc
tree, build a reference graph from the Sphinx directives, roles and
toctrees, then list every text and image file that no longer has a path
from a documentation root.

Usage:
    ./devtools/check-doc-orphans.py
    ./devtools/check-doc-orphans.py --all --verbose

Returns non-zero exit code if orphaned files are found, suitable for CI.
"""
```

#### 2. Function docstrings could specify types
**File:** Multiple functions

While the code is correct, adding parameter and return type hints would improve maintainability and align with modern Python practices (Python 3.5+ type hints).

**Example for `resolve` function:**
```python
def resolve(target: str, path: Path, srcdir: Path) -> Iterator[Path]:
    """
    Yield the files one target can refer to.
    
    Args:
        target: The reference target string from a directive/role
        path: The file containing the reference
        srcdir: The Sphinx source directory
        
    Yields:
        Resolved Path objects that the target may refer to
    """
```

This is a **minor improvement** suggestion, not a requirement, since DPDK doesn't mandate type hints for Python scripts.

#### 3. Regex pattern construction readability
**File:** `devtools/check-doc-orphans.py:47-49`

The `PATH_RE` regex construction is functionally correct but could be slightly more readable:

**Current:**
```python
PATH_RE = re.compile(
    r"[\w./-]+\.(?:" + "|".join(s[1:] for s in sorted(CANDIDATE_SUFFIXES)) + r")\b"
)
```

**Suggested:** Add a comment explaining the pattern:
```python
# Match file paths ending in candidate suffixes (e.g., .svg, .png, .rst)
PATH_RE = re.compile(
    r"[\w./-]+\.(?:" + "|".join(s[1:] for s in sorted(CANDIDATE_SUFFIXES)) + r")\b"
)
```

---

### **Info (Consider)**

#### 1. Error handling completeness
**File:** `devtools/check-doc-orphans.py:74-77`

The `OSError` handling in `targets()` is good, but consider whether partial file read errors should be handled differently:

```python
try:
    text = path.read_text(encoding="utf-8", errors="replace")
except OSError as err:
    sys.exit(f"{path}: {err.strerror}")
```

The `errors="replace"` will silently substitute invalid UTF-8 sequences. For a documentation checker, this is probably acceptable (documentation should be valid UTF-8), but you might want to at least warn if replacement occurs. This is low priority since malformed UTF-8 in doc files would be caught elsewhere.

#### 2. Boolean logic clarity in line 163
**File:** `devtools/check-doc-orphans.py:163`

The condition could use parentheses for clarity, though operator precedence is correct:

**Current:**
```python
if hit.suffix in CANDIDATE_SUFFIXES and not hit.is_file():
```

**Suggested (optional):**
```python
if (hit.suffix in CANDIDATE_SUFFIXES) and (not hit.is_file()):
```

This is purely stylistic; the current form is acceptable.

---

## Positive Observations

1. **Good separation of concerns:** Each function has a single, clear responsibility
2. **Proper resource management:** Uses context-managed file operations via `Path.read_text()`
3. **No correctness bugs identified:** No resource leaks, race conditions, or logic errors
4. **Appropriate use of standard library:** Good use of `pathlib.Path`, `argparse`, and `re`
5. **Exit codes:** Correctly returns non-zero on finding issues (suitable for CI)
6. **Character encoding:** Explicitly specifies UTF-8 encoding and error handling
7. **Configurability:** Good use of `IGNORE` set for intentional exceptions

---

## Correctness Review

**No correctness bugs found.** The code properly:
- Handles file I/O errors
- Uses generators efficiently for large file sets
- Avoids infinite loops with the `seen` set in `walk_graph()`
- Correctly resolves relative paths using `Path.resolve()`
- Validates directory existence before proceeding

---

## Process and Format

- **Python style:** Complies with black formatting (4-space indentation, line length <=100)
- **SPDX license:** Correct BSD-3-Clause header
- **Copyright:** Current year (2026), proper format
- **Script permissions:** Marked executable (`#!/usr/bin/env python3`)

---

## Overall Assessment

**This is a well-written utility script with no correctness issues.** The suggested improvements are minor enhancements to documentation and style, not functional defects. The code is suitable for merging after the contributor considers the warnings above.

**Recommendation:** ACCEPT with minor suggestions.