Repository navigation
Read gemspec directives and dev deps as Ruby code (#1932) - #1936
Conversation
Assisted-by: Claude Code:claude-opus-5-5
for_each_code_line becomes code_lines, an iterator that also yields the 0-based line index. gemspec_directives, dev_dependency_requirements and locked_exact_requirement's gem-line lookup now skip =begin/=end blocks, heredoc bodies and __END__ data, as Ruby does, instead of scanning raw lines. A commented-out dev dependency no longer pins '~> 2, = 1.0.0' where Bundler locks '= 1.0.0', and a heredoc body no longer refuses the Gemfile. Fixes #1932 Assisted-by: Claude Code:claude-opus-5-5
The gem-declaration lookup reads the comment-stripped code like the other code_lines readers, and bundler_merges_dev_dependencies keeps trimming stray CRs from the BUNDLED WITH header. Assisted-by: Claude Code:claude-opus-5-5
|
BugBot review Generated by Claude Code |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit ba7b17d. Configure here.
Assisted-by: Claude Code:claude-opus-5-5
|
Ready for review (burn-down agent).
Generated by Claude Code |
Final review briefWhat it does: The two gemspec readers in Risk: Low. The change is limited to gem manifest parsing. The iterator conversion is mechanical for the 3 existing callers. Line indices keep their meaning ( Look here:
Verified: Read the full diff and all callers of Changes I made: none. Open questions: none. The description lists the follow-ups ( Auto-merge (squash) is armed, so approving sends it straight to the merge queue. Generated by Claude Code |
Wenxin Jiang (Wenxin-Jiang)
left a comment
There was a problem hiding this comment.
[agent] Reviewed ba7b17d4c32427b9be68975fb525b47a30db868b. Checked shared Ruby code-line iteration through gemspec directives, dev dependencies and VEX. No major correctness issues found. Validation: source and regression-test review plus the available exact-head CI checks; no local test pass is claimed.
LLM Description written by Claude Code:claude-opus-5-5
Fixes #1932
Summary
Register row A54 (see Part 3, duplicated business logic).
formats/gem/manifest.rsalready has a shared Ruby code-line reader that skips=begin/=endblocks, heredoc bodies and__END__data, andvex/product.rsuses it to read gemspecs. The two gemspec readers informats/gem/gemspec.rsscanned raw lines instead, so:add_development_dependencywas merged into the pin (~> 2, = 1.0.0where Bundler locks= 1.0.0), and frozen installs failed;gemspec path: ROOTinside a heredoc body refused the whole Gemfile.This PR drops the per-reader line scanning and routes every reader through the one shared reader.
Changes
for_each_code_line→code_lines(manifest.rs). The callback becomes an iterator that also yields the 0-based line index, so readers that returnErrearly can use?. Its 3 existing callers (gemfile_lockfile,eval_gemfile_paths,vex::product::parse_gemspec) become plainforloops or acollect.gemspec_directivesiteratescode_lines. The directive'slineindex has the same meaning as before.dev_dependency_requirementsiteratescode_lines. Its own#check is gone, because the reader now matches on code only.locked_exact_requirement'sgem-line lookup iteratescode_linestoo. Otherwise agemline inside=beginwould count as the declaration that the directive has to come before.begin_blocks_heredocs_and_end_data_are_not_codecovers a directive in=begin, a heredoc and after__END__; a dev dependency in=beginand in a heredoc; and agemline in=begin. Red→green: withmain's readers restored and this test kept, it fails withleft: Err("line 5: \gemspec path: ROOT`")`. With this PR it passes.Size
git diff --stat: 3 files, +55/−31. That is about +33/−31 production lines and +22 test lines.Behavior change
Only for Gemfiles and gemspecs with
=beginblocks, heredocs,__END__data or a leading BOM. Those lines are no longer read asgemspecdirectives, dev dependencies orgemdeclarations, as the issue's acceptance criteria require. Everything else is unchanged.Verification
cargo clippy --workspace --all-features -- -D warnings: clean.cargo test -p socket-patch-core --lib: 6996 passed. The 4 failures are the known root-sandbox ones that also fail onmain:copy_treesymlinked root,vlt_healunremovable lock,pypi_poetryandpypi_requirementswire failures.in_process_gem_apply,in_process_gem_multi_platformande2e_vexpass./code-review high
9 findings. I fixed 2:
gem-line lookup now matches oncodelike the other readers;\rtrim inbundler_merges_dev_dependencies(removing it was not behavior-neutral).Not fixed, with reasons:
gemline inside aninstance_eval <<~Gheredoc is now missed. Ruby does evaluate it, but the issue asks for heredoc bodies to be skipped, andgemfile_lockfileandvexalready treat them that way.heredoc_idsmisreadsx <<FLAG(a shift) as a heredoc; an indented terminator closes a plain<<IDheredoc. Both are existing heuristics of the shared reader, whichgemfile_lockfileand VEX already depend on. Fixing them is a separate change tomanifest.rs.vendor/gem.rsplan_gemfile_edit,vex/discover/gem.rsandredirect/upstream/gem.rsstill hand-scan=begin. For the new test's Gemfile, the writer refuses ("declared more than once"), which fails closed. Routing them throughcode_linesis a follow-up, noted in the register.code_ofre-run on stripped code,Vec::remove(0). All are negligible. Thecode_ofre-run is what still detects an unterminated quote.Risk
Low: one file's readers, plus a mechanical signature change in two callers. No
cfg(windows)/cfg(unix)code was touched.https://claude.ai/code/session_011f9aePo7nZ74KqPGHC4rvE
Generated by Claude Code