Sync pragma-annotated additional_dependencies with uv.lock - #56
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #56 +/- ##
==========================================
+ Coverage 98.34% 98.80% +0.45%
==========================================
Files 4 5 +1
Lines 181 250 +69
Branches 35 44 +9
==========================================
+ Hits 178 247 +69
Misses 2 2
Partials 1 1 ☔ View full report in Codecov by Harness. |
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The dependency-line regexes (_DEP_LINE_RE, _DEP_BARE_RE, _DEP_EXTRA_RE) are fairly permissive and may misclassify more complex PEP 508 cases (e.g. markers with commas, URLs, non-standard specifiers); consider tightening these patterns or clearly constraining the supported syntax to avoid surprising false positives or rejections.
- process_config_text now raises a generic ValueError for pragma-related issues; introducing a dedicated exception type (e.g. SyncWithUvError) would make it easier for callers to distinguish configuration errors from other failures and handle them appropriately.
- _DEP_EXTRA_RE currently treats any additional comma-separated token between the specifier and the pragma as a second dependency; you may want to refine this to avoid rejecting lines that contain commas in markers or other non-dependency syntax in that segment.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The dependency-line regexes (_DEP_LINE_RE, _DEP_BARE_RE, _DEP_EXTRA_RE) are fairly permissive and may misclassify more complex PEP 508 cases (e.g. markers with commas, URLs, non-standard specifiers); consider tightening these patterns or clearly constraining the supported syntax to avoid surprising false positives or rejections.
- process_config_text now raises a generic ValueError for pragma-related issues; introducing a dedicated exception type (e.g. SyncWithUvError) would make it easier for callers to distinguish configuration errors from other failures and handle them appropriately.
- _DEP_EXTRA_RE currently treats any additional comma-separated token between the specifier and the pragma as a second dependency; you may want to refine this to avoid rejecting lines that contain commas in markers or other non-dependency syntax in that segment.
## Individual Comments
### Comment 1
<location path="tests/test_prek.py" line_range="283-289" />
<code_context>
+ }
+
+
+def test_sync_additional_dependencies_toml_bare_adds_specifier() -> None:
+ """A bare pragma dependency in prek.toml gets an exact pin added."""
+ prek_text = textwrap.dedent("""\
+ [[repos.hooks]]
+ id = "mypy"
+ additional_dependencies = [
+ "pydantic", # sync-with-uv
+ 'attrs>=1', # sync-with-uv
+ ]
+ """)
+ uv_data = {"pydantic": "2.5.0", "attrs": "23.2.0"}
+
+ result, changes = process_config_text(prek_text, uv_data, config_format="toml")
+
+ assert ' "pydantic==2.5.0", # sync-with-uv' in result
+ assert " 'attrs==23.2.0', # sync-with-uv" in result
+ assert changes.repos == {}
+ assert changes.lines == {
+ 4: ("pydantic", "", "==2.5.0"),
+ 5: ("attrs", ">=1", "==23.2.0"),
</code_context>
<issue_to_address>
**suggestion (testing):** You might want a TOML test that covers inserting pins before environment markers, similar to the YAML case.
The YAML test `test_sync_additional_dependencies_bare_adds_specifier` already verifies that pins are inserted before environment markers (e.g. `; python_version < "3.11"`), but the TOML tests don’t cover a dependency line with a marker. Consider adding a TOML array entry like `"types-PyYAML ; python_version < \"3.11\"" # sync-with-uv` and asserting that the pin is inserted before the marker to mirror the YAML coverage and lock in the PEP 508 behavior for TOML as well.
```suggestion
assert changes.lines == {
4: ("pydantic", "", "==2.5.0"),
5: ("attrs", ">=1", "==23.2.0"),
}
def test_sync_additional_dependencies_toml_bare_adds_specifier_with_marker() -> None:
"""A bare pragma dependency with a marker in prek.toml pins before the marker."""
prek_text = textwrap.dedent("""\
[[repos.hooks]]
id = "mypy"
additional_dependencies = [
"types-PyYAML ; python_version < \"3.11\"", # sync-with-uv
]
""")
uv_data = {"types-pyyaml": "6.0.1"}
result, changes = process_config_text(prek_text, uv_data, config_format="toml")
assert " \"types-PyYAML==6.0.1 ; python_version < \\\"3.11\\\"\", # sync-with-uv" in result
assert changes.repos == {}
assert changes.lines == {
4: ("types-pyyaml", "", "==6.0.1"),
}
def test_sync_additional_dependencies_toml_errors() -> None:
```
</issue_to_address>
### Comment 2
<location path="src/sync_with_uv/sync_with_uv.py" line_range="11" />
<code_context>
from sync_with_uv.repo_data import repo_to_package, repo_to_version_template
+# A dependency line is only synced when it carries this pragma comment,
+# e.g. ``- pydantic==2.0.0 # sync-with-uv``. The pragma is an explicit,
+# per-line opt-in, so the sync is safe regardless of where the line lives.
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the dependency parsing and sync logic into fewer, more cohesive primitives (shared regex, unified result type, and clearer helpers) to make the code easier to follow and maintain.
A few focused refactors would reduce complexity while preserving all current behavior.
---
### 1. Simplify dependency parsing regexes
You can collapse `_DEP_LINE_RE` and `_DEP_BARE_RE` into a single pattern with an optional spec group, and handle “bare” vs “pinned” in Python. This also lets you drop `_DEP_EXTRA_RE` by doing a simple post‑split.
Example:
```python
# Single pattern with optional specifier
_DEP_ITEM_RE = re.compile(
rf"""
^
(?P<prefix>\s*(?:-\s+['"]?|['"]))
(?P<name>[A-Za-z0-9][A-Za-z0-9._-]*)
(?P<extras>\[[^\]]*\])?
\s*
(?P<spec>{_DEP_SPEC})? # optional specifier
(?P<rest>.*)$ # everything after the first dependency
""",
re.VERBOSE,
)
def _parse_dependency(line: str) -> tuple[str, str, int, int] | str:
m = _DEP_ITEM_RE.match(line)
if not m:
return "no dependency to sync"
name = m.group("name")
spec = m.group("spec") or ""
spec_span = (m.start("spec"), m.end("spec")) if spec else (m.end("extras") or m.end("name"),) * 2
# Reject extra dependencies with a simple split instead of another regex
rest = m.group("rest")
if "," in rest.split("#", 1)[0]:
return "more than one dependency on the line; use one per line"
return name, spec, *spec_span
```
Then `_sync_dependency_line` can use `_parse_dependency` instead of managing three regexes and a separate `_DEP_EXTRA_RE`.
---
### 2. Use a single structured result type for dependency lines
`_DepSyncResult | str | None` forces type checks and a separate error path. You can keep all functionality but simplify the API by returning a single result type with a status flag, or by raising for invalid pragma lines.
**Option A: status-based result (no raises)**
```python
class DepSyncOutcome(NamedTuple):
line: str
package: str | None
old_spec: str
new_spec: str
error: str | None
def _sync_dependency_line(line: str, uv_data: dict[str, str]) -> DepSyncOutcome | None:
pragma = _DEP_PRAGMA_RE.search(line)
if not pragma:
return None
parsed = _parse_dependency(line)
if isinstance(parsed, str):
return DepSyncOutcome(line=line, package=None, old_spec="", new_spec="", error=parsed)
name, old_spec, spec_start, spec_end = parsed
package = _normalize_package_name(name)
if package not in uv_data:
return DepSyncOutcome(
line=line, package=package, old_spec=old_spec, new_spec="", error=f"{package!r} is not in uv.lock"
)
target_spec = f"=={uv_data[package]}"
line_fixed = line[:spec_start] + target_spec + line[spec_end:]
return DepSyncOutcome(line=line_fixed, package=package, old_spec=old_spec, new_spec=target_spec, error=None)
```
Caller usage:
```python
elif (dep_result := _sync_dependency_line(line, uv_data)) is not None:
if dep_result.error:
dep_errors.append(f"line {line_number}: {dep_result.error}")
new_lines.append(line)
else:
dep_changes[line_number] = DepLineChange(
dep_result.package, dep_result.old_spec, dep_result.new_spec
)
new_lines.append(dep_result.line)
continue
```
This keeps the “collect errors then raise” behavior but removes `str | NamedTuple | None` from the signature.
**Option B: raise on invalid pragma**
If you prefer simpler control flow, `_sync_dependency_line` can raise `ValueError` for invalid pragma lines and return `None | DepLineChange`, letting `process_config_text` catch and aggregate:
```python
def _sync_dependency_line(line: str, uv_data: dict[str, str]) -> DepLineChange | None:
pragma = _DEP_PRAGMA_RE.search(line)
if not pragma:
return None
# ... parse dependency, raise ValueError on invalid cases ...
return DepLineChange(package, old_spec, new_spec)
```
```python
elif _DEP_PRAGMA_RE.search(line):
try:
dep_change = _sync_dependency_line(line, uv_data)
except ValueError as exc:
dep_errors.append(f"line {line_number}: {exc}")
new_lines.append(line)
else:
if dep_change is not None:
dep_changes[line_number] = dep_change
new_lines.append(dep_change.line)
continue
```
---
### 3. Collapse `_DepSyncResult` and `DepLineChange`
Currently `_DepSyncResult` and `DepLineChange` carry nearly the same information. You can drop `_DepSyncResult` and let `DepLineChange` be the single representation, including the line if you wish:
```python
class DepLineChange(NamedTuple):
line: str
package: str
old_spec: str
new_spec: str
# _sync_dependency_line returns DepLineChange | None (or DepSyncOutcome in Option A)
dep_changes[line_number] = dep_result # no need to re-wrap
```
This simplifies both `_sync_dependency_line` and the `Changes` type signatures.
---
### 4. Separate repo and dependency paths in `process_config_text`
To keep `process_config_text` linear, you can split the dependency sync into a helper that operates on lines and returns updated lines plus `dep_changes`/errors. This reduces nesting in the main loop.
```python
def _sync_dependencies_in_lines(
lines: list[str], uv_data: dict[str, str]
) -> tuple[list[str], dict[int, DepLineChange], list[str]]:
new_lines: list[str] = []
dep_changes: dict[int, DepLineChange] = {}
dep_errors: list[str] = []
for line_number, line in enumerate(lines, start=1):
dep_result = _sync_dependency_line(line, uv_data)
if dep_result is None:
new_lines.append(line)
continue
if dep_result.error:
dep_errors.append(f"line {line_number}: {dep_result.error}")
new_lines.append(line)
else:
dep_changes[line_number] = DepLineChange(
dep_result.package, dep_result.old_spec, dep_result.new_spec
)
new_lines.append(dep_result.line)
return new_lines, dep_changes, dep_errors
```
Then `process_config_text` first handles repo headers/revs, then calls the helper:
```python
lines = config_text.splitlines(keepends=True)
lines_after_repo, repo_changes = _sync_repo_revs(lines, uv_data, ...)
lines_after_dep, dep_changes, dep_errors = _sync_dependencies_in_lines(lines_after_repo, uv_data)
if dep_errors:
msg = "invalid '# sync-with-uv' dependencies:\n " + "\n ".join(dep_errors)
raise ValueError(msg)
return "".join(lines_after_dep), Changes(repo_changes, dep_changes)
```
This keeps all current functionality (including aggregated error reporting and per-line `DepLineChange`), but makes each path easier to understand and modify in isolation.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| assert changes.lines == { | ||
| 4: ("pydantic", "", "==2.5.0"), | ||
| 5: ("attrs", ">=1", "==23.2.0"), | ||
| } | ||
|
|
||
|
|
||
| def test_sync_additional_dependencies_toml_errors() -> None: |
There was a problem hiding this comment.
suggestion (testing): You might want a TOML test that covers inserting pins before environment markers, similar to the YAML case.
The YAML test test_sync_additional_dependencies_bare_adds_specifier already verifies that pins are inserted before environment markers (e.g. ; python_version < "3.11"), but the TOML tests don’t cover a dependency line with a marker. Consider adding a TOML array entry like "types-PyYAML ; python_version < \"3.11\"" # sync-with-uv and asserting that the pin is inserted before the marker to mirror the YAML coverage and lock in the PEP 508 behavior for TOML as well.
| assert changes.lines == { | |
| 4: ("pydantic", "", "==2.5.0"), | |
| 5: ("attrs", ">=1", "==23.2.0"), | |
| } | |
| def test_sync_additional_dependencies_toml_errors() -> None: | |
| assert changes.lines == { | |
| 4: ("pydantic", "", "==2.5.0"), | |
| 5: ("attrs", ">=1", "==23.2.0"), | |
| } | |
| def test_sync_additional_dependencies_toml_bare_adds_specifier_with_marker() -> None: | |
| """A bare pragma dependency with a marker in prek.toml pins before the marker.""" | |
| prek_text = textwrap.dedent("""\ | |
| [[repos.hooks]] | |
| id = "mypy" | |
| additional_dependencies = [ | |
| "types-PyYAML ; python_version < \"3.11\"", # sync-with-uv | |
| ] | |
| """) | |
| uv_data = {"types-pyyaml": "6.0.1"} | |
| result, changes = process_config_text(prek_text, uv_data, config_format="toml") | |
| assert " \"types-PyYAML==6.0.1 ; python_version < \\\"3.11\\\"\", # sync-with-uv" in result | |
| assert changes.repos == {} | |
| assert changes.lines == { | |
| 4: ("types-pyyaml", "", "==6.0.1"), | |
| } | |
| def test_sync_additional_dependencies_toml_errors() -> None: |
d7d14c2 to
1f23793
Compare
Add opt-in syncing of dependency lines (such as additional_dependencies entries) to uv.lock. A line is processed only when it carries a `# sync-with-uv` pragma comment; its package is then pinned to the exact uv.lock version: an existing specifier (`==`, `>=`, `~=`, ...) is rewritten to `==<locked>`, and a bare dependency with no specifier has an `==<locked>` pin added. The pragma-per-line approach is format-agnostic (works in both .pre-commit-config.yaml and prek.toml) and safe by default, since arbitrary additional_dependencies are never touched without an explicit opt-in. Because the pragma is an explicit request to sync a line, an annotated line that cannot be synced is an error (exit code 123) rather than a silent no-op: - the package is not present in uv.lock (typically a typo or forgotten add), - the line has no dependency to sync (the pragma is on a comment or a non-dependency line such as a hook `id`), or - the line has more than one dependency (only one per pragma line is supported, so the rest are never silently skipped). All offending lines are reported together with their line numbers, and the config file is left unchanged until they are fixed. process_config_text now returns a Changes(repos, lines) result: repo `rev` syncs are reported per package (as before) and dependency pins are reported per line number, so the same package pinned on several lines is reported once per line instead of collapsing to a single entry. The CLI summary gains a "N dependencies changed, M dependencies left unchanged" line when any pragma dependency is present, and both summary lines pluralize by count. Package names are matched against uv.lock after PEP 503 normalization. Extras, environment markers, quoting, and the comment itself are preserved. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FB7BB52sP6N6AGpbFTbB4v
Code-quality cleanup of the pragma dependency-sync path: - Drop the near-duplicate `_DepSyncResult` type; `_sync_dependency_line` now returns `(updated_line, DepLineChange)` directly, removing a field-by-field re-copy in the caller. - Extract `_replace_span()` for the "splice a computed value into a line span" idiom shared by the rev path and the dependency path. - Add a `DepLineChange.changed` property instead of re-deriving `old_spec != new_spec` in three places. - Share the PEP 508 name subpattern (`_DEP_NAME`) between the two dependency regexes and drop unused capture groups. Also fixes a correctness gap in the "one dependency per line" guard: the old comma-based heuristic falsely rejected an environment marker containing a comma (e.g. `pydantic>=1.0; extra == "a,b"`) and missed space-separated dependencies. Replace it with a whitelist of the text allowed after a specifier (whitespace, quotes, commas, then an optional `;` marker), which both fixes the false positive and catches space-separated dependencies. Adds tests for the marker case, space-separated rejection, and dependency-line line-ending preservation. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FB7BB52sP6N6AGpbFTbB4v
Move the `# sync-with-uv` dependency-line handling out of sync_with_uv.py into a new dependency_line.py leaf module, mirroring how repo_data.py isolates the repo-URL mapping concern. The module holds the `_DEP_*` regexes, DepLineChange, _normalize_package_name, and the now-public sync_dependency_line entry point; sync_with_uv.py imports them and keeps the Changes result type plus the process_config_text orchestrator. This trims sync_with_uv.py from ~300 to ~170 lines. Also inline the short `_replace_span` helper at its two call sites (the rev path and the dependency path) now that it no longer needs to be shared across modules, and update CLAUDE.md's project structure and key-functions list. No behaviour change; the existing tests (which all exercise process_config_text) pass unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FB7BB52sP6N6AGpbFTbB4v
b46a329 to
7b69ddb
Compare
Add two tests closing behavioral gaps in the `# sync-with-uv` dependency syncing: - `test_cli_reports_unchanged_dependency_line` exercises the verbose per-line "unchanged" report (cli.py) for a dependency already at the locked version, including the singular "1 dependency" pluralization. - `test_sync_additional_dependencies_pragma_lookalike_ignored` pins the intent of the `(?![\w-])` lookahead: `# sync-with-uv-experimental`, `# sync-with-uvx` and `# sync-with-uv2` must not trigger syncing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KGSHUhV4XnPnDfz7QXZjey
Mirror the YAML marker coverage on the prek.toml path: a bare `# sync-with-uv` dependency with an environment marker gets its pin inserted before the marker. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014rnrE5NEAJyEAyr7PveJjc
Add opt-in syncing of dependency lines (such as
additional_dependenciesentries) touv.lock. A line is processed only when it carries a# sync-with-uvpragma comment; its package is then pinned to the exact version fromuv.lock: an existing specifier (==,>=,~=, ...) is rewritten to an exact==pin, and a bare dependency with no specifier has one added.The pragma-per-line approach is format-agnostic (works in both
.pre-commit-config.yamlandprek.toml) and safe by default, since arbitraryadditional_dependenciesare never touched without an explicit opt-in.Because the pragma is an explicit request to sync a line, an annotated line that cannot be synced is an error (exit code 123) rather than a silent no-op:
uv.lock(typically a typo or a forgotten add),id), orAll offending lines are reported together with their line numbers, and the config file is left unchanged until they are fixed.
process_config_textnow returns aChanges(repos, lines)result: reporevsyncs are reported per package (as before) and dependency pins are reported per line number, so the same package pinned on several lines is reported once per line instead of collapsing to a single entry. The CLI summary gains a "N dependencies changed, M dependencies left unchanged" line when any pragma dependency is present, and both summary lines pluralize by count.Package names are matched against
uv.lockafter PEP 503 normalization. Extras, environment markers, quoting, and the comment itself are preserved.🤖 Generated with Claude Code
https://claude.ai/code/session_01FB7BB52sP6N6AGpbFTbB4v