Conversation
Walkthrough
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🧰 Additional context used🧬 Code graph analysis (1)lib/rewrite/source.ex (3)
🔇 Additional comments (7)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Pull Request Test Coverage Report for Build eb4132a758e2346393d76df22c2fbf02525e38a0Details
💛 - Coveralls |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/rewrite/source.ex (1)
935-943: Fix the typo in theSource.format/2specThe spec now advertises
{:errror, term()}, which doesn’t match the actual{:error, term()}return and will mislead Dialyzer/users. Please correct the atom.- @spec format(t(), opts()) :: {:ok, t()} | {:errror, term()} + @spec format(t(), opts()) :: {:ok, t()} | {:error, term()}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
mix.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
.github/workflows/ci.yml(3 hunks)lib/rewrite.ex(14 hunks)lib/rewrite/dot_formatter.ex(3 hunks)lib/rewrite/source.ex(8 hunks)mix.exs(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
lib/rewrite/source.ex (2)
lib/rewrite.ex (1)
do_update(596-613)lib/rewrite/source/ex.ex (2)
handle_update(144-146)handle_update(149-161)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
.github/workflows/ci.yml (3)
92-93: Make coverage env explicit.Avoid coupling to mix cli/0; run coverage in test env explicitly.
- run: mix coveralls.github + run: MIX_ENV=test mix coveralls.github
14-33: Add Elixir 1.19 to the matrix when available.This PR targets 1.19 readiness; include 1.19.x (or RC) once setup-beam supports it. Optionally set
continue-on-error: trueuntil GA.
1-1: Optional: add workflow concurrency and disable matrix fail-fast.Prevents duplicate runs on rapid pushes and ensures all matrix jobs run.
name: CI +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true jobs: linux: name: Test on Ubuntu (Elixir ${{ matrix.elixir }}, OTP ${{ matrix.otp }}) runs-on: ubuntu-24.04 strategy: + fail-fast: false matrix:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/ci.yml(3 hunks)CHANGELOG.md(1 hunks)mix.exs(2 hunks)
🔇 Additional comments (2)
CHANGELOG.md (1)
3-7: Changelog entry looks good; remember to sync the tag/version on release.Dev header and notes align with the code changes. When tagging 1.2.0, bump @Version and source_ref accordingly.
mix.exs (1)
16-16: Min Elixir bump aligns with PR goal.Matches the CHANGELOG note. CI matrix includes 1.14.5, so coverage looks consistent.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/rewrite_test.exs (1)
1114-1121: Consider testing path ordering more explicitly.The test creates sources in alphabetical path order (a.exs, b.exs, c.exs), which means it doesn't fully validate that
Enum.to_list/1returns sources ordered by path. If the implementation returned sources in insertion order instead, this test would still pass.For comparison, the "sources/1" test at lines 872-884 creates sources out of order (c, a, b) and verifies they're returned in path order (a.exs, b.exs, c.exs), which better validates the ordering behavior.
Apply this diff to create sources out of order and strengthen the test:
test "to_list/1" do a = Source.from_string(":a", path: "a.exs") b = Source.from_string(":b", path: "b.exs") c = Source.from_string(":c", path: "c.exs") - {:ok, project} = Rewrite.from_sources([a, b, c]) + {:ok, project} = Rewrite.from_sources([c, a, b]) assert Enum.to_list(project) == [a, b, c] end
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/rewrite/source.ex(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
lib/rewrite/source.ex (2)
lib/rewrite.ex (2)
do_update(596-613)format(858-861)lib/rewrite/source/ex.ex (3)
handle_update(144-146)handle_update(149-161)format(283-293)
🔇 Additional comments (3)
lib/rewrite/source.ex (3)
104-104: LGTM!The hash type change to
non_neg_integer()correctly reflects the implementation using:erlang.phash2/1(line 826).
21-32: LGTM!The struct expansion with
timestamp,from,owner, andprivatefields is well-designed and properly documented. The type spec correctly reflects all new fields.Also applies to: 101-112
529-543: LGTM!Tightening the pattern matching to explicit
%Source{}structs in the helper functions improves type safety and prevents accidentally passing incorrect data structures to these internal functions.
1 participant
No description provided.