chore: introduce num42_refactors + apply safe AST refactors#30
Open
aspala wants to merge 38 commits into
Open
Conversation
devenv.nix and CI already use Elixir 1.19; the mix.exs pin was lagging at ~> 1.16. Bump to ~> 1.18 to match the actual runtime.
Adds the num42/num42_refactors Mix task (sourceror-driven AST refactors) as a dev-only git dep. Library is pre-release and not yet on hex.pm, so pinned to the github main branch. .refactor.exs declares the input globs (lib/, test/) with empty configured_modules and skipped_modules — refactors will be enabled incrementally as they prove safe on this codebase. Invocation: `mix refactor --dry-run` to preview, `mix refactor` to apply. See https://github.com/num42/num42_refactors for the full refactor catalog.
Applied via `mix refactor --only ExpandShortFormBindings`. Renames single-letter and abbreviated local bindings to their unambiguous long forms, so the data flow is self-describing instead of requiring readers to trace context. Concretely: - ctx → context (3 files) - fp → fingerprint (similarity.ex) - ed → edit_distance (candidates.ex) - pct → percent (distance.ex) - third → target (bradford.ex) - id → identifier (vowel_density.ex) - entry → name (collector.ex walk loop) - avg → score (grader.ex) - base_m/head_m → base/head (delta.ex) Three sites (cli/health_report.ex, formatter/github.ex, formatter/plain.ex) required manual repair after the refactor: the tool collapsed distinct bindings (base_val + head_val, avg_node_us + tokens_per_node_us) into a single shadowed name, silently breaking semantics. Manual rename to distinct long forms (base_value/head_value, avg_node_us/per_node_us, avg_us) restores the intended behavior. 884 tests, 0 failures.
The refactor renames short bindings to a fixed long-form whitelist without checking whether the target name already exists in scope. On this codebase it collapses distinct bindings (e.g. base_val/head_val, avg_node_us/ tokens_per_node_us) into a single shadowed name, silently breaking semantics. It also fights manual rename decisions on every rerun (avg_us → microseconds → microseconds collision). Disabled in skipped_modules until the upstream behavior is scope-aware.
Applied via `mix refactor --only ExpandShortFormFunctions` with manual
overrides where the tool's whitelist picked semantically wrong long forms.
- yaml_formatter.ex: fmt_scalar → format_scalar
(fmt is a format verb; the tool's "formatter_scalar" reads as a noun)
- formatter/plain.ex: pl(n, word) → pluralize(n, word)
(pl = pluralize helper; the tool's "plain" shadows the parent module
name Formatter.Plain and obscures intent)
- 4 signal classification tests: defp t(...) → defp token(...)
(the tool's "test" shadows ExUnit's test/2 macro and breaks compilation)
- data_signal_test.exs: defp str(v) → defp str_token(v)
(the tool's "stream" is semantically unrelated — str = string-token)
884 tests, 0 failures.
ExpandShortFormFunctions ran a second pass on str_token and would rename it to stream_token (still semantically wrong — these are <STR>-kind token maps, not a stream). Renaming to string_kind_token is unambiguous and escapes the tool's `str → stream` whitelist mapping.
Applied via `mix refactor --only AliasUsage`. Where modules reference a deep FQN (e.g. CodeQA.AST.Lexing.NewlineToken.kind()) the long path is now lifted into a single `alias` at the module top and call sites become NewlineToken.kind(). Concentrates dependency info, easier to skim, easier to refactor when the dependency moves. 34 files touched. The tool places aliases immediately after `defmodule` (before @moduledoc) — that disagrees with the existing convention in this repo (moduledoc first), but is technically equivalent and the upstream tool's placement is left as-is. 884 tests, 0 failures.
Applied via `mix refactor --only LiftDirectives`. Both formatter modules (plain.ex, github.ex) had `alias CodeQA.HealthReport.BehaviorLabels` repeated inside two private functions; lifting to the module top removes the duplicates and surfaces the dependency once. The tool's initial placement put the alias between `@spec render` and `def render`, which would have orphaned the spec — moved manually to the correct slot (after @moduledoc, before module attributes). 884 tests, 0 failures.
Applied via `mix refactor --only MultiAliasExpand`. Splits compact
`alias Foo.{A, B, C}` declarations into one `alias` per line.
Trade-off: longer files vs. grep-ability and per-alias diffability.
`alias Foo.C` is now a literal grep hit, and adding/removing one alias
no longer churns the entire group's diff.
18 files touched. 884 tests, 0 failures.
Applied via `mix refactor --only PipeReassign`. Where a variable is reassigned by passing it as the first arg to a single function call, the refactor rewrites the form as a pipe so the data flow reads left-to-right. Note: the style guide (common/coding-style.md) warns against single-step pipes, and most of these hits are exactly that. Trade-off accepted — readability is subjective and the codebase has prior pipe- heavy style, so consistency wins here. Multi-step chains will benefit on follow-ups as more transformations get added. 11 files touched. 884 tests, 0 failures.
Applied via `mix refactor --only ExtractToPipeline`. Wherever the first
argument of an `Enum.*` / `Stream.*` call was a bare variable, the form
is rewritten as `var |> Enum.fn(...)` so the data flow reads left-to-right.
87 files touched.
The tool incorrectly rewrote one site in
test/codeqa/metrics/file/near_duplicate_blocks_test.exs:
Enum.sum(for d <- 0..8, do: result["near_dup_block_d#{d}"])
became
for d <- 0..8, do: result["near_dup_block_d#{d}"] |> Enum.sum()
which changes semantics — the pipe binds to a single integer inside the
comprehension instead of summing the list. Reverted that hunk by hand.
No other for/comprehension sites were affected.
884 tests, 0 failures.
🟠 Code Health: C (61/100)
%%{init: {'theme': 'neutral'}}%%
xychart-beta
title "Code Health Scores"
x-axis ["Readability", "Complexity", "Structure", "Duplication", "Naming", "Magic Numbers", "Combined Metrics"]
y-axis "Score" 0 --> 100
bar [95, 31, 87, 48, 96, 100, 63]
|
🔍 Top Likely Issues (cosine similarity)
🟢 Readability — A (95/100)Codebase averages: flesch_adapted=97.64, fog_adapted=4.81, avg_tokens_per_line=9.45, avg_line_length=35.65
🔴 Complexity — D- (31/100)Codebase averages: difficulty=41.12, effort=234510.50, volume=4034.50, estimated_bugs=1.34
🟢 Structure — A- (87/100)Codebase averages: branching_density=0.14, mean_depth=3.88, avg_function_lines=8.43, max_depth=9.20, max_function_lines=20.42, variance=6.94, avg_param_count=1.14, max_param_count=1.98
🟠 Duplication — C- (48/100)Codebase averages: redundancy=0.59, bigram_repetition_rate=0.54, trigram_repetition_rate=0.37
🟢 Naming — A (96/100)Codebase averages: entropy=0.89, mean=6.68, variance=19.13, avg_sub_words_per_id=1.18
🟢 Magic Numbers — A (100/100)Codebase averages: density=0.00
🔴 Combined Metrics — D (63/100)
🔴 Code Smells — D (39/100)
🟠 Consistency — C- (50/100)
🔴 Dependencies — D- (27/100)
🟡 Documentation — B+ (83/100)
🟢 Error Handling — A- (92/100)
🔴 File Structure — D+ (42/100)
🟡 Function Design — B+ (82/100)
🟡 Naming Conventions — B+ (82/100)
🔴 Scope And Assignment — D- (28/100)
🟡 Testing — B+ (83/100)
🟢 Type And Value — A- (90/100)
🟡 Variable Naming — B (74/100)
|
🔴 lib/codeqa/metrics/file/inflector.ex:32-45 — No Implicit Null InitialIssues:
32 │ """
33 │ @spec detect_casing(String.t()) ::
34 │ :pascal_case | :camel_case | :snake_case | :macro_case | :kebab_case | :other
35 │ def detect_casing(identifier) do
36 │ cond do
37 │ identifier =~ ~r/^[A-Z][a-zA-Z0-9]*$/ -> :pascal_case
38 │ identifier =~ ~r/^[a-z][a-z0-9]*(?:[A-Z][a-zA-Z0-9]*)+$/ -> :camel_case
39 │ identifier =~ ~r/^[a-z]+(_[a-z0-9]+)*$/ -> :snake_case
40 │ identifier =~ ~r/^[A-Z]+(_[A-Z0-9]+)*$/ -> :macro_case
41 │ identifier =~ ~r/^[a-z]+(-[a-z0-9]+)*$/ -> :kebab_case
42 │ true -> :other
43 │ end
44 │ end
45 │ end
🔴 lib/codeqa/cli/health_report.ex:1-14 — Negated Boolean NameIssues:
1 │ defmodule CodeQA.CLI.HealthReport do
2 │ @moduledoc false
3 │
4 │ @behaviour CodeQA.CLI.Command
5 │
6 │ alias CodeQA.CLI.Options
7 │ alias CodeQA.Config
8 │ alias CodeQA.Engine.Analyzer
9 │ alias CodeQA.Engine.Collector
10 │ alias CodeQA.Git
11 │ alias CodeQA.HealthReport
12 │
13 │ @impl CodeQA.CLI.Command
14 │ def usage do🔴 lib/codeqa/cli/health_report.ex:43-54 — Negated Boolean NameIssues:
43 │ @command_options [
44 │ output: :string,
45 │ config: :string,
46 │ detail: :string,
47 │ top: :integer,
48 │ format: :string,
49 │ ignore_paths: :string,
50 │ base_ref: :string,
51 │ head_ref: :string,
52 │ telemetry: :boolean,
53 │ comment: :boolean
54 │ ]🔴 lib/codeqa/cli/health_report.ex:97-102 — Negated Boolean NameIssues:
97 │ |> Map.put("metadata", %{
98 │ "path" => Path.expand(path),
99 │ "timestamp" => DateTime.utc_now() |> DateTime.to_iso8601(),
100 │ "total_files" => map_size(files),
101 │ "total_bytes" => total_bytes
102 │ })🔴 lib/codeqa/cli/health_report.ex:97-102 — Negated Boolean NameIssues:
97 │ |> Map.put("metadata", %{
98 │ "path" => Path.expand(path),
99 │ "timestamp" => DateTime.utc_now() |> DateTime.to_iso8601(),
100 │ "total_files" => map_size(files),
101 │ "total_bytes" => total_bytes
102 │ })🔴 lib/codeqa/cli/health_report.ex:136-143 — Negated Boolean NameIssues:
136 │ HealthReport.generate(results,
137 │ config: opts[:config],
138 │ detail: detail,
139 │ top: top_n,
140 │ base_results: base_results,
141 │ changed_files: changed_files,
142 │ diff_line_ranges: diff_line_ranges
143 │ )🔴 lib/codeqa/cli/health_report.ex:184-188 — Negated Boolean NameIssues:
184 │ |> Enum.each(fn {content, n} ->
185 │ path = Path.join(tmpdir, "codeqa-part-#{n}.md")
186 │ File.write!(path, content)
187 │ IO.puts(:stderr, "Part #{n} written to #{path} (#{byte_size(content)} bytes)")
188 │ end)🔴 lib/codeqa/cli/health_report.ex:236-250 — Negated Boolean NameIssues:
236 │ Agent.start_link(fn ->
237 │ %{
238 │ codebase_cosines_us: 0,
239 │ codebase_metrics: %{},
240 │ cosine_breakdown: %{},
241 │ cosine_breakdown_calls: 0,
242 │ file_metrics: %{},
243 │ files: [],
244 │ loo_breakdown: %{},
245 │ loo_breakdown_calls: 0,
246 │ nodes: [],
247 │ phases: %{},
248 │ stages: %{}
249 │ }
250 │ end)🔴 lib/codeqa/cli/health_report.ex:237-249 — Negated Boolean NameIssues:
237 │ %{
238 │ codebase_cosines_us: 0,
239 │ codebase_metrics: %{},
240 │ cosine_breakdown: %{},
241 │ cosine_breakdown_calls: 0,
242 │ file_metrics: %{},
243 │ files: [],
244 │ loo_breakdown: %{},
245 │ loo_breakdown_calls: 0,
246 │ nodes: [],
247 │ phases: %{},
248 │ stages: %{}
249 │ }🔴 lib/codeqa/cli/health_report.ex:252-266 — Negated Boolean NameIssues:
252 │ :telemetry.attach_many(
253 │ @telemetry_handler,
254 │ [
255 │ [:codeqa, :block_impact, :codebase_cosines],
256 │ [:codeqa, :block_impact, :file],
257 │ [:codeqa, :block_impact, :node],
258 │ [:codeqa, :stage],
259 │ [:codeqa, :file_metric],
260 │ [:codeqa, :codebase_metric],
261 │ [:codeqa, :loo_breakdown],
262 │ [:codeqa, :cosine_breakdown]
263 │ ],
264 │ &handle_event(&1, &2, &3, &4),
265 │ pid
266 │ ) |
Applied via `mix refactor --only ExtractToPipeline` second iteration. Catches sites that became eligible after the first pass (e.g. nested `Enum.map` inside a `flat_map`) plus a handful that were missed. 12 files touched. Same upstream bug recurred at near_duplicate_blocks_test.exs:195 — the tool keeps rewriting `Enum.sum(for d <- 0..8, do: ...)` into a broken comprehension- pipe form. Reverted by hand and rewrote the line in pipe form (`0..8 |> Enum.map(...) |> Enum.sum()`) so it no longer matches the refactor's trigger pattern. Confirmed with `--dry-run`: 0 files would change. 884 tests, 0 failures.
Applied via `mix refactor --only MapNewLambdaToForComprehension`. Where
the lambda body is a bare 2-tuple, rewrites
Map.new(coll, fn x -> {k, v} end)
to
for x <- coll do
{k, v}
end
|> Map.new()
Trade-off: verbosity (one line → three) for explicit comprehension shape.
The for-comp form makes filters/guards (`for x <- coll, cond?, do: ...`)
trivial to add later, and the trailing `|> Map.new()` keeps the "build a
map" intent visible at the end of the pipeline.
10 files touched. 884 tests, 0 failures.
Applied via `mix refactor --only EnumCapture`. Where a lambda in an
Enum/Stream HOF just calls a single function with the bound variable,
the explicit form is replaced with the capture form:
fn x -> f(x) end → &f/1
fn x -> &1 != y end → &(&1 != y)
fn x -> g(x, k) end → &g(&1, k)
Reverted two sites where the tool produced multi-line &-captures that
read worse than the original:
* block_impact_analyzer.ex#196 — 8-arg call wrapped in `&(...)` with
only one captured slot. Kept the explicit `fn node -> ...` form.
* formatter/plain.ex#58 — multi-line capture for a 2-arg call. Single-
line `fn cat -> render_category(cat, detail) end` is clearer.
20 files touched. 884 tests, 0 failures.
The previous iteration reverted these two sites because the &-capture form spans multiple lines and reads worse than the explicit `fn x -> ...` form when only one of 8 (or 2) call args is captured. The refactor keeps re-applying the change on every run, however — there's no "leave-as-explicit" annotation to honor. Accepting the &-capture form here so subsequent iterations don't loop on these two sites. Tests are green, the trade-off is purely stylistic. * block_impact_analyzer.ex#196 — 8-arg `serialize_node(&1, ...)` * formatter/plain.ex#59 — 2-arg `render_category(&1, detail)` 884 tests, 0 failures.
Applied via `mix refactor --only EnumReduceToSum`. Two sites with manual sum-via-reduce (`Enum.reduce(coll, 0, fn x, acc -> acc + f(x) end)`) collapsed to `Enum.sum_by(coll, fn x -> f(x) end)`. Same semantics, named operation. 2 files touched. 884 tests, 0 failures.
Applied via `mix refactor --only ReduceMapPut`. One site rewriting
`Enum.reduce(coll, %{}, fn x, acc -> Map.put(acc, k, v) end)` to
`Map.new(coll, fn x -> {k, v} end)`. Same semantics, named constructor.
1 file touched. 884 tests, 0 failures.
…rved
Applied via `mix refactor --only EnumIntoToMapNew`. Replaces
`... |> Enum.into(%{})` with `Map.new(...)`. Semantics identical;
Map.new is the dedicated constructor and removes the "into empty map vs
merge into existing" ambiguity.
The tool wrote the result as `Map.new(<pipe chain>)` which broke the
left-to-right pipe flow. Manually rewrote both sites as the final pipe
step:
files
|> Flow.from_enumerable(...)
|> Flow.map(...)
|> Map.new()
target_paths
|> Map.new(fn path -> ... end)
2 files touched. 884 tests, 0 failures.
Applied via `mix refactor --only MapGetUnsafePass`. `Keyword.get(opts, :k, nil)` collapses to `Keyword.get(opts, :k)` — the 2-arg form already returns nil for missing keys. Same semantics, less noise. 2 files touched. 884 tests, 0 failures.
Applied via `mix refactor --only MapNewToPipe`. `Map.new(coll)` rewritten as `coll |> Map.new()`. Style-only — same semantics, matches the codebase's pipe-first convention even when the pipe is single-step. 3 files touched. 884 tests, 0 failures.
Applied via `mix refactor --only UseMapJoin`. Replaces list |> Enum.map(fn) |> Enum.join(sep) with the fused single-pass form list |> Enum.map_join(sep, fn) The tool wrote two sites as `Enum.map_join(<pipe chain>, sep, fn)` which broke the pipeline. Manually rewrote them as the final pipe step. 2 files touched. 884 tests, 0 failures.
Applied via `mix refactor --only ExtractToPipeline`. After UseMapJoin produced `Enum.map_join(1..20, "\n", fn)` in git_test.exs the next pass converts the first arg into pipe form: `1..20 |> Enum.map_join(...)`. 1 file touched. 884 tests, 0 failures.
Applied via `mix refactor --only CaseTrueFalse`. One site collapsed `case bool do true -> a; false -> b end` to the if/else form. Shorter syntax, intent visible at a glance, no risk of non-exhaustive match. 1 file touched. 884 tests, 0 failures.
Applied via `mix refactor --only ExtractParametricClone` plus significant
manual repair — the tool produced structurally broken output and the
result here is mostly hand-rewritten.
Three shared modules introduced:
* CodeQA.AST.Nodes.Shared — `cast_shared/2`. The six AST node modules
(Code/Doc/Function/Import/Module/Test) each had an identical 6-field
`cast(%Node{})` that built `%__MODULE__{...}`. Consolidated into one
helper that takes the target struct module so the same code can
populate any of them via `struct/2`.
* CodeQA.Shared — `project_languages_shared/1` and
`humanize_category_shared/1`. The former was duplicated across
HealthReport, BlockImpactAnalyzer, and Diagnostics; the latter
across SampleRunner and Grader.
* CodeQA.HealthReport.Formatter.Shared — `count_severities_shared/1`
and `worst_severity_shared/1`, identical in Github and Plain
formatters.
Manual repairs needed on top of the tool's output:
* The tool wrote new files under `lib/code_qa/` (with underscore) but
the existing convention is `lib/codeqa/`. Moved all three.
* `Number42.Refactors.Ex.ExtractParametricClone` emitted
`def cast_shared(node), do: %__MODULE__{...}` — but `__MODULE__` in
the shared module is `CodeQA.AST.Nodes.Shared`, which has no struct,
so compilation failed. Rewrote as `cast_shared(target_struct, node)`
using `struct(target_struct, fields)`, and updated all six callsites
from `cast_shared(node)` to `cast_shared(__MODULE__, node)`.
* `project_languages_shared` was emitted as nested-call form; rewrote
with pipes to match the rest of the codebase.
* `count_severities_shared` was one long line; broken across pipes.
* Every callsite had the `import ..., only: [...]` placed before
`@moduledoc` with broken indentation; moved to the conventional
position and removed the now-unused `alias CodeQA.Language` from
HealthReport.
* Sample_runner.ex `import` got dropped during the position-fix and
had to be re-added.
13 files modified, 3 new shared modules. 884 tests, 0 failures.
Applied via `mix refactor --only EnumCapture`. The shared helper introduced in the previous commit used the explicit lambda form, which this iteration rewrites as a capture. Style-only. 1 file touched. 884 tests, 0 failures.
Applied via `mix refactor --only AliasOrder`. Two test files had aliases in source-order rather than alphabetical; sorted so subsequent inserts land at their canonical position without churning the surrounding hunk. 2 files touched. 884 tests, 0 failures.
Applied via `mix refactor --only ExtractCaseToHelper`. Where a `case` on a function-call result sat as the tail expression of a def body, the dispatch is lifted into a private multi-clause helper. Caveats: the tool emits the helpers with generic placeholder names (`handle_load_configs_ls`, `handle_get_in`, `handle_next_token`, ...) and inserts a FIXME comment asking for a better name. The indentation of multi-line bodies inside the helpers is also off; mix format cleaned most of it up. Accepting as-is rather than hand-renaming all 7 helpers — the mechanical change is what the tool offers, and we'll judge case-by-case on follow-ups whether each extraction is worth keeping. 7 files touched. 884 tests, 0 failures.
Applied via `mix refactor --only ExtractCaseToHelper`. Catches more case-on-call dispatches uncovered by the first pass. Same caveats as the previous commit: generic helper names, FIXME comments, manual review desirable per site. 4 files touched. 884 tests, 0 failures.
One more case-on-call dispatch site in git.ex. 1 file touched. 884 tests, 0 failures.
Applied via `mix refactor --only ExtractSharedModule`. Both formatters (Github, Plain) carried an identical `defp pr_summary_section/1`; the tool removed both private copies and updated the imports to expect a public `pr_summary_section/1` in Formatter.Shared. Tool bug: the tool removed the function from both callers but did NOT add it to the Shared module. Manually added the @doc + @SPEC + body to Formatter.Shared. Also deleted a duplicate `lib/code_qa/` (with underscore) stub the tool re-created in the wrong path. Additionally rolls in a sample-sync drift on priv/combined_metrics/{code_smells,variable_naming}.yml that the test suite produced earlier in this session (adds `_languages:` keys). 3 files (formatter github + plain + shared), 2 YAML refreshes. 884 tests, 0 failures.
Applied via `mix refactor --only ImportAfterAlias`. Moves module-top `import` statements to come after the `alias` block, matching the canonical Elixir `use → alias → import → require` order. 13 files touched. 884 tests, 0 failures.
Applied via `mix refactor --only IfElseToCond` on
lib/codeqa/ast/signals/classification/data_signal.ex; the result
broke 2 tests.
The tool rewrote
if next == nil do
if total > 0 and ratio > 0.6, do: vote, else: nothing
else
nothing
end
into a cond block whose first arm read `compute_total.() -> vote` —
losing both the outer `next == nil` guard and the inner ratio
threshold. Any input with `total > 0` cast a vote, including the
control-flow and low-ratio cases the tests check.
Reverted the data_signal.ex change and disabled the refactor in
skipped_modules. Re-enable once the upstream behavior preserves
guards on outer `if` branches.
884 tests, 0 failures.
Applied via `mix refactor --only IfLiftToClauses`. Functions whose body is `if cond, do: X, else: Y` get rewritten as two pattern-matched clauses, with the condition expressed as a head pattern or guard. Manual fix in lib/codeqa/health_report/grader.ex:overall_score/3 — the tool emitted both new clauses with the original defaults, which is illegal Elixir (defaults can only appear in one clause). Added a default-only header clause and dropped the defaults from the two implementation clauses. 4 files touched. 884 tests, 0 failures.
Applied via `mix refactor --only InlineSingleExpressionDef`. Defs whose entire body is one expression collapse from `def hello do :world end` to `def hello, do: :world`. The tool also rewrites multi-arg first-positional calls into single-step pipes (e.g. `do: __MODULE__ |> GenServer.start_link(opts)`) — conflicts with the global "no single-step pipes" guideline but matches the tool's stated intent. Multi-line pipes in do: position are syntactically valid but visually awkward. Mix format left them as-is. 75 files touched. 884 tests, 0 failures.
The refactor sorts every def/defp group alphabetically across the entire
module. On this codebase that produces a 121-file diff at once and:
- deletes section comments (`# --- GenServer callbacks ---`,
`# --- Private helpers ---`) that mark structural intent
- mixes public API, impl callbacks, and private helpers into one
alphabetical block, ignoring the public-first / impl-next /
private-last convention used throughout
- separates same-name clauses (the second hit on `lib/codeqa/cli/
health_report.ex` triggered `clauses with the same name and arity
should be grouped together`)
Tests still pass, but the diff is unreviewable and the imposed order
disagrees with the existing structural organisation. Disabled in
skipped_modules.
Applied via `mix refactor --only SortKeywords`. Sorts the contents of defstruct fields, struct/map literals, and pattern matches so a new key lands at its sort position rather than at the bottom or "wherever the author was working." Stabilises diffs in code review. The tool emits left-aligned key lines; `mix format` cleans them up to the canonical indentation, so the on-disk result is tidy. 87 files touched. 884 tests, 0 failures.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Introduces
num42/num42_refactorsas a dev-only Mix dep and applies its full AST-refactor catalog to the codebase in a reviewable sequence. Each refactor is its own commit; unsafe refactors are explicitly skipped via.refactor.exs.Final state:
mix refactor --dry-runreports 0 files would change. 884 tests passing at every commit.Setup commits
chore: bump Elixir pin to ~> 1.18— devenv/CI already on 1.19,mix.exswas lagging at~> 1.16chore(deps): add num42_refactors as dev dep + config— pinned to the GitHubmainbranch (lib is pre-release, not on hex)Applied refactors (chronological)
base_val/head_val) into shadowed namestest/2or used semantically wrong namesaliasto module topalias Foo.{A, B}→ one alias per linex = f(x, ...)→x = x |> f(...)Enum.f(x, ...)→x |> Enum.f(...). Manual fix on afor/do: |> Enum.sumsite the tool got wrongMap.new(coll, fn)→ for-comp pipelinefn x -> f(x) end→&f/1Enum.sum_by/2Map.new/2Enum.into(%{})→Map.new. Manually rewroteMap.new(<chain>)calls into|> Map.new()to keep the pipe flowMap.new(coll)→coll |> Map.new()map + jointomap_joincase bool do true -> a; false -> b end→if/elsecasedispatches to multi-clause helpers. Tool emits generic names + FIXME comments — accepted as-ispr_summary_sectiontoFormatter.Shared(tool removed callers but didn't add the body — manual add)importafteraliasif cond, do: X, else: Y→ two pattern-matched clauses. Manual fix inGrader.overall_score/3where the tool emitted illegal duplicate defaultsdefbodies collapse todo:formdefstruct/ struct / map / pattern-match keys alphabeticallySkipped refactors (
skipped_modulesin.refactor.exs)Three refactors were disabled because they produced unsafe or unreviewable output:
if next == nil do (if total > 0 and ratio > 0.6 ...)into acondwhose first arm cast the vote oncompute_total.()alone. Broke 2 tests on first try.Each skip is documented with its rationale in the
.refactor.exsfile.How to verify
mix test— 884 tests, 0 failuresmix compile --warnings-as-errors— cleanmix refactor --dry-run— 0 files would change (refactor loop is at fixpoint)Notes for review
x = x \|> f(...),if list \|> Enum.empty?()...) — conflicts mildly with the global style guide ("avoid single-step pipes") but matches the codebase's pipe-first convention.aliasplaced before@moduledocin modules touched byAliasUsage— the existing convention is moduledoc first, but the tool's placement is left as-is to keep the diff to what the tool produced. Files touched byExtractParametricClonewere manually fixed to moduledoc-first because they were already disturbed.ExtractCaseToHelpergeneric helper names (handle_load_configs_ls,handle_get_in, ...) with FIXME comments — manual renaming was deferred.Test plan
mix testpassesmix compile --warnings-as-errorspassesmix refactor --dry-runshows 0 files would change.refactor.exsshould remain skipped