Skip to content

Comments

feat: case-insensitive input/ matching and test improvements#92

Merged
athola merged 3 commits intomainfrom
small-updates-0.2.5
Feb 22, 2026
Merged

feat: case-insensitive input/ matching and test improvements#92
athola merged 3 commits intomainfrom
small-updates-0.2.5

Conversation

@athola
Copy link
Owner

@athola athola commented Feb 20, 2026

Summary

Fixes #86, fixes #89, fixes #90

Case-insensitive input/ directory matching for cross-platform filesystem support, test consolidation via parametrization, and edge case coverage for non-YAML files.

Changes

Test Plan

  • 19 tests pass in test_infer_data_dir.py (including 3 new parametrized case-variant tests)
  • New parametrized case-insensitive dir test (Input, INPUT, iNpUt) in both test files
  • New non-YAML edge case test confirms ValueError
  • All pre-commit hooks pass (ruff, mypy, bandit, pytest, architecture checks)
  • CI: 10/11 checks pass, 1 pending (Modified File Coverage)

Use .lower() comparison for input/ directory detection in core.py and
io_utils.py so that Input/, INPUT/, etc. trigger the grandparent logic
on case-insensitive filesystems.

Collapse three extension-specific tests into a single parametrized test
and add edge case coverage for non-YAML files in input/.

Fixes #86
Fixes #89
Fixes #90
Bump version 0.2.4 -> 0.2.5 across pyproject.toml, __init__.py,
uv.lock, wiki API references, and CHANGELOG. Includes case-insensitive
input/ directory matching fix and parametrized test improvements.
@athola
Copy link
Owner Author

athola commented Feb 21, 2026

PR Review: #92 - Case-Insensitive input/ Matching

Verdict: APPROVE

Scope: All 12 changed files (+94/-39) are in scope for fixes #86, #89, #90 plus release prep.


Implementation Analysis

Core Changes (3 locations, all correct):

  • core.py:553base_dir.name.lower() == "input" (warning path)
  • core.py:572parent.name.lower() == "input" (grandparent resolution)
  • io_utils.py:65candidate.parent.name.lower() == "input" (read path resolution)

No missed locations — exhaustive search for == "input" comparisons confirmed these are the only three.

Pattern consistency.lower() approach matches existing convention for file extensions (e.g., .suffix.lower() in _YAML_SUFFIXES at core.py:563).

Test Quality

Parametrization — Well-structured consolidation of 3 separate tests into 1 parametrized test for file extensions. Clean ["Input", "INPUT", "iNpUt"] variants for directory name case testing.

Edge cases covered:

  • Case-variant directory names (3 variants x 2 code paths)
  • Non-YAML file in input/ raises ValueError
  • Warning emitted for case-variant explicit data_dir
  • io_utils.py path resolution with case variants

Version Consistency

Version 0.2.5 is consistent across pyproject.toml, __init__.py, uv.lock, CHANGELOG.md, and wiki docs. No stale 0.2.4 references.

Documentation Quality

Slop score: 2/10 (clean). PR description, CHANGELOG, README, and wiki additions use factual, human-developer voice.

CI Status

10/11 checks passing. Modified File Coverage pending.

Non-Blocking Suggestions

  1. Nested case variant coveragetest_yaml_in_nested_input_dir_returns_grandparent only tests lowercase input. Could parametrize to include INPUT variant. (Optional, low priority)

Blocking Issues

None.

@athola
Copy link
Owner Author

athola commented Feb 21, 2026

Test Plan

  • Modified File Coverage CI check passes (currently pending)
  • pytest tests/unit/shell/test_infer_data_dir.py -v — all 19 tests pass including parametrized case variants
  • pytest tests/unit/shell/test_io_utils.py -v — case-variant input dir test passes
  • Manual: create directory named INPUT/ with a .yaml file, verify _infer_data_dir_and_name returns grandparent
  • Verify no regressions in full test suite: pytest --tb=short

@athola
Copy link
Owner Author

athola commented Feb 21, 2026

Comprehensive PR Review (pr-review-toolkit)

Agents run: code-reviewer, pr-test-analyzer, silent-failure-hunter, comment-analyzer, code-simplifier


Critical Issues (0 found)

None. No bugs, security vulnerabilities, or blocking defects in the changed code.


Important Issues (2 found)

1. [silent-failure-hunter] resolve_paths_for_read has no guard against root-level parent.parent

  • File: src/simple_resume/shell/io_utils.py:64-66
  • Severity: HIGH (pre-existing, expanded by this PR)
  • Issue: If candidate is Path('/input/resume.yaml'), then candidate.parent.parent yields Path('/'). Unlike core.py, there is no exists() gate here. The Paths object silently resolves to /output/ for writes.
  • Recommendation: Add an explicit guard:
    if candidate.parent.name.lower() == "input":
        grandparent = candidate.parent.parent
        if grandparent == Path("/") or grandparent == Path("."):
            raise ValueError(...)
        base_dir = grandparent

2. [comment-analyzer] Docstrings not updated for case-insensitive behavior

  • File: src/simple_resume/shell/generate/core.py:530-548 — docstring says input/ but doesn't mention case-insensitivity
  • File: src/simple_resume/shell/io_utils.py:57 — single-line docstring with no mention of input/ logic at all
  • Recommendation: Update docstrings to note that input/ matching is case-insensitive (e.g., Input/, INPUT/)

Suggestions (5 found)

3. [silent-failure-hunter] core.py:572-573 — root-level parent.parent yields Path('/')

4. [silent-failure-hunter] Warning at core.py:551-559 is suppressible

  • The UserWarning about path-doubling can be silenced via Python warning filters. Code then proceeds with the known-bad path. Consider raising ValueError instead of warning.

5. [code-simplifier] Merge duplicate warning tests

  • test_infer_data_dir.py:186-225: test_warns_when_data_dir_named_input and test_warns_when_data_dir_named_input_case_insensitive are near-duplicates (~18 lines of redundancy). Merge into one parametrized test with ["input", "Input", "INPUT", "iNpUt"].

6. [comment-analyzer] CHANGELOG issue attribution

7. [pr-test-analyzer] Minor test coverage gaps

  • test_infer_data_dir.py:47-61: Nested input dir test only uses lowercase input. Consider parametrizing with ["input", "INPUT"].
  • test_io_utils.py:92-103: Could also assert result.input == input_dir to verify original casing is preserved.

Strengths

  • All three .lower() change sites have direct, targeted parametrized test coverage
  • Consistent ["Input", "INPUT", "iNpUt"] variant set used across all test sites
  • Clean test consolidation: 3 separate tests → 1 parametrized test (extension variants)
  • BDD story.given/when/then pattern maintained consistently
  • Non-YAML edge case test (test_non_yaml_file_in_input_dir_raises) is a valuable addition
  • Version 0.2.5 is consistent across all 6 version-bearing files
  • Documentation is slop-free (2/10 score), professional voice throughout
  • All 55 tests pass, 10/11 CI checks green

Recommended Action

  1. Consider adding root-path guard to io_utils.py:64-66 (Important feat: add comprehensive testing suite and CI/CD pipeline #1) — this is pre-existing but the .lower() change expands its surface area
  2. Consider updating docstrings in core.py and io_utils.py (Important Migrate from inline styles to external CSS files #2) — prevents comment rot
  3. Optional merge duplicate warning tests (Suggestion Feature: add wide variety of generated custom color schemes #5) — saves ~18 lines
  4. All other suggestions are non-blocking improvements for future work

Overall verdict: This PR is safe to merge. The important items are documentation/defense-in-depth concerns, not correctness bugs.

- Add root-path guard to resolve_paths_for_read for degenerate
  parent.parent when input/ is at filesystem root
- Update docstrings to document case-insensitive input/ matching
- Merge duplicate warning tests into single parametrized test
- Fix CHANGELOG attribution: separate test issues (#89, #90) from
  path-resolution fix (#86)

Deferred to issues: #93, #94, #95
@athola
Copy link
Owner Author

athola commented Feb 22, 2026

PR Review Fixes Applied (c3119c7)

Fixed Items

ID Description Evidence
I1 Root-path guard in resolve_paths_for_read commit c3119c7 + new test
I2 Docstrings updated for case-insensitive behavior commit c3119c7
S3 Merged duplicate warning tests into parametrized test commit c3119c7
S4 CHANGELOG attribution separated (#86 vs #89/#90) commit c3119c7

Deferred to Issues

ID Description Issue
S1 Defense-in-depth root guard in core.py #93
S2 ValueError vs UserWarning for path-doubling #94
S5 Parametrize nested test + assert result.input #95

Informational (No Action)

  • Reviewer verdict "APPROVE" / "safe to merge" — None

@athola athola merged commit 1adfb1a into main Feb 22, 2026
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant