From fcbf4e5c4ca4ed59d37dbdf6ca64637ed46724ac Mon Sep 17 00:00:00 2001 From: Neil Carvalho Date: Mon, 25 May 2026 16:49:48 -0300 Subject: [PATCH 1/3] Switch Structs for the Data class --- lib/commands.rb | 2 +- lib/config.rb | 4 ++-- lib/executor.rb | 2 +- lib/parsers/audit_parser.rb | 2 +- lib/parsers/outdated_parser.rb | 2 +- lib/planner.rb | 12 +++--------- lib/reconciler.rb | 4 ++-- test/fixture_runner.rb | 2 +- 8 files changed, 12 insertions(+), 18 deletions(-) diff --git a/lib/commands.rb b/lib/commands.rb index d6c754b..eea61f5 100644 --- a/lib/commands.rb +++ b/lib/commands.rb @@ -19,7 +19,7 @@ module Update # safer (no shell-injection surprises from package names) and easier to # match against fixture keys. module Commands - Result = Struct.new(:stdout, :stderr, :exit_code, keyword_init: true) do + Result = Data.define(:stdout, :stderr, :exit_code) do def success? exit_code == 0 end diff --git a/lib/config.rb b/lib/config.rb index 3d62c8e..1425ece 100644 --- a/lib/config.rb +++ b/lib/config.rb @@ -24,7 +24,7 @@ class Config VALID_STRATEGIES = %i[grouped individual].freeze - Bucket = Struct.new(:strategy, keyword_init: true) do + Bucket = Data.define(:strategy) do def grouped? strategy == :grouped end @@ -34,7 +34,7 @@ def individual? end end - CommitMessage = Struct.new(:prefix, keyword_init: true) + CommitMessage = Data.define(:prefix) attr_reader :version, :grouping, diff --git a/lib/executor.rb b/lib/executor.rb index 046229f..7f845f1 100644 --- a/lib/executor.rb +++ b/lib/executor.rb @@ -40,7 +40,7 @@ def failed? end end - Report = Struct.new(:outcomes, :warnings, keyword_init: true) + Report = Data.define(:outcomes, :warnings) def initialize( gh:, diff --git a/lib/parsers/audit_parser.rb b/lib/parsers/audit_parser.rb index 411a966..6023f9f 100644 --- a/lib/parsers/audit_parser.rb +++ b/lib/parsers/audit_parser.rb @@ -20,7 +20,7 @@ module Parsers # free-form text. If it ever contains a literal `|`, we rejoin the # overflow cells so the description survives intact. class AuditParser - Vulnerability = Struct.new(:name, :severity, :vulnerable_versions, :advisory, keyword_init: true) + Vulnerability = Data.define(:name, :severity, :vulnerable_versions, :advisory) SEVERITIES = %w[low moderate high critical].freeze diff --git a/lib/parsers/outdated_parser.rb b/lib/parsers/outdated_parser.rb index 5137ea2..3c1ea12 100644 --- a/lib/parsers/outdated_parser.rb +++ b/lib/parsers/outdated_parser.rb @@ -21,7 +21,7 @@ module Parsers # underlying OutdatedPackage. Those rows are returned with `error: ...` # set and `latest: nil`, so callers can decide whether to skip them. class OutdatedParser - OutdatedPackage = Struct.new(:name, :current, :latest, :error, keyword_init: true) do + OutdatedPackage = Data.define(:name, :current, :latest, :error) do def parseable? !latest.nil? end diff --git a/lib/planner.rb b/lib/planner.rb index 86af6c8..39019a0 100644 --- a/lib/planner.rb +++ b/lib/planner.rb @@ -37,24 +37,18 @@ module Update # attention than patches. class Planner # An individual package update slated for inclusion in some PR. - PackageBump = Struct.new( - :name, :from, :to, :semver_kind, :advisory, - keyword_init: true - ) + PackageBump = Data.define(:name, :from, :to, :semver_kind, :advisory) # A planned pull request. The reconciler (step 4) will compare these against # existing open PRs on GitHub to decide what to open, force-push, or close. - PRSpec = Struct.new( - :kind, :packages, :branch, :title, :metadata, - keyword_init: true - ) do + PRSpec = Data.define(:kind, :packages, :branch, :title, :metadata) do def security? kind == :security end end # The full output of a planning pass. - Plan = Struct.new(:pr_specs, :warnings, keyword_init: true) do + Plan = Data.define(:pr_specs, :warnings) do def empty? pr_specs.empty? end diff --git a/lib/reconciler.rb b/lib/reconciler.rb index 7632924..36bc92b 100644 --- a/lib/reconciler.rb +++ b/lib/reconciler.rb @@ -31,7 +31,7 @@ module Update class Reconciler # Input describing an open PR that GitHub returned. Step 5's client # will fill these in from the list-PRs API call. - ExistingPR = Struct.new(:number, :branch, :body, :title, keyword_init: true) + ExistingPR = Data.define(:number, :branch, :body, :title) Action = Struct.new(:type, :pr_spec, :existing_pr, :reason, keyword_init: true) do def open? @@ -51,7 +51,7 @@ def noop? end end - Result = Struct.new(:actions, :ignored, keyword_init: true) do + Result = Data.define(:actions, :ignored) do def opens actions.select(&:open?) end diff --git a/test/fixture_runner.rb b/test/fixture_runner.rb index a3a37b1..c441c96 100644 --- a/test/fixture_runner.rb +++ b/test/fixture_runner.rb @@ -6,7 +6,7 @@ module Importmap module Update module Commands class FixtureRunner - Fixture = Struct.new(:pattern, :result, keyword_init: true) + Fixture = Data.define(:pattern, :result) def initialize(fixtures = []) @fixtures = fixtures From a16b74400ea4710f25344db319be5e2f21e1803e Mon Sep 17 00:00:00 2001 From: Neil Carvalho Date: Mon, 25 May 2026 17:11:02 -0300 Subject: [PATCH 2/3] Use hash shorthand syntax --- exe/importmap-update | 8 ++++---- lib/commands.rb | 2 +- lib/config.rb | 12 ++++++------ lib/git_client.rb | 2 +- lib/github_client.rb | 2 +- lib/parsers/audit_parser.rb | 8 ++++---- lib/parsers/outdated_parser.rb | 4 ++-- lib/planner.rb | 6 +++--- test/executor_test.rb | 22 +++++++++++----------- test/fixture_runner.rb | 4 ++-- test/planner_test.rb | 16 ++++++++-------- test/reconciler_test.rb | 26 +++++++++++++------------- 12 files changed, 56 insertions(+), 56 deletions(-) diff --git a/exe/importmap-update b/exe/importmap-update index 5beadea..a4e4b3f 100755 --- a/exe/importmap-update +++ b/exe/importmap-update @@ -67,7 +67,7 @@ end rails_root = ENV.fetch("RAILS_ROOT", ".") runner = Importmap::Update::Commands::ShellRunner.new(cwd: rails_root) -gh = Importmap::Update::GitHubClient.new(repo: repo, token: token) +gh = Importmap::Update::GitHubClient.new(repo:, token:) git = Importmap::Update::GitClient.new( repo: Git.open(rails_root), author_name: ENV.fetch("IMPORTMAP_AUTHOR_NAME", "github-actions[bot]"), @@ -77,14 +77,14 @@ git = Importmap::Update::GitClient.new( outdated = Importmap::Update::Parsers::OutdatedParser.parse(ENV.fetch("IMPORTMAP_OUTDATED_OUTPUT")) vulnerabilities = Importmap::Update::Parsers::AuditParser.parse(ENV.fetch("IMPORTMAP_AUDIT_OUTPUT")) plan = Importmap::Update::Planner.new( - outdated: outdated, vulnerabilities: vulnerabilities, config: config + outdated:, vulnerabilities:, config: ).call existing_prs = gh.list_open_prs(branch_prefix: config.branch_prefix) -reconciled = Importmap::Update::Reconciler.new(plan: plan, existing_prs: existing_prs).call +reconciled = Importmap::Update::Reconciler.new(plan:, existing_prs:).call executor = Importmap::Update::Executor.new( - gh: gh, git: git, runner: runner, + gh:, git:, runner:, base_branch: ENV.fetch("IMPORTMAP_BASE_BRANCH", "main"), commit_message_prefix: config.commit_message.prefix, labels: config.labels, diff --git a/lib/commands.rb b/lib/commands.rb index eea61f5..8eff1e2 100644 --- a/lib/commands.rb +++ b/lib/commands.rb @@ -47,7 +47,7 @@ def run(*argv) opts[:chdir] = @cwd if @cwd Bundler.with_unbundled_env do stdout, stderr, status = Open3.capture3(*argv, opts) - Result.new(stdout: stdout, stderr: stderr, exit_code: status.exitstatus) + Result.new(stdout:, stderr:, exit_code: status.exitstatus) end end diff --git a/lib/config.rb b/lib/config.rb index 1425ece..e33acc5 100644 --- a/lib/config.rb +++ b/lib/config.rb @@ -62,13 +62,13 @@ def self.default def to_h { - version: version, + version:, grouping: grouping.transform_values { |b| {strategy: b.strategy} }, - open_pull_requests_limit: open_pull_requests_limit, - labels: labels, - reviewers: reviewers, - commit_message: {prefix: commit_message.prefix}, - branch_prefix: branch_prefix + open_pull_requests_limit:, + labels:, + reviewers:, + branch_prefix:, + commit_message: {prefix: commit_message.prefix} } end diff --git a/lib/git_client.rb b/lib/git_client.rb index b443050..a52d30b 100644 --- a/lib/git_client.rb +++ b/lib/git_client.rb @@ -47,7 +47,7 @@ def commit_changes(message:) end def push(branch:, force: false) - @repo.push("origin", branch, force: force) + @repo.push("origin", branch, force:) nil end end diff --git a/lib/github_client.rb b/lib/github_client.rb index 5e536b2..01921db 100644 --- a/lib/github_client.rb +++ b/lib/github_client.rb @@ -62,7 +62,7 @@ def ensure_labels(labels) # Edits an existing PR's title and body. def update_pr(number:, title:, body:) - @client.update_pull_request(@repo, number, title: title, body: body) + @client.update_pull_request(@repo, number, title:, body:) nil end diff --git a/lib/parsers/audit_parser.rb b/lib/parsers/audit_parser.rb index 6023f9f..575cfaf 100644 --- a/lib/parsers/audit_parser.rb +++ b/lib/parsers/audit_parser.rb @@ -62,11 +62,11 @@ def split_row(line) # If a description contained a `|`, cells.size will be >4. Rejoin the # tail into the advisory column so we don't lose information. def build_row(cells) - name, severity, vuln_versions, *advisory_parts = cells + name, severity, vulnerable_versions, *advisory_parts = cells Vulnerability.new( - name: name, - severity: severity, - vulnerable_versions: vuln_versions, + name:, + severity:, + vulnerable_versions:, advisory: advisory_parts.join(" | ") ) end diff --git a/lib/parsers/outdated_parser.rb b/lib/parsers/outdated_parser.rb index 3c1ea12..e6de8e8 100644 --- a/lib/parsers/outdated_parser.rb +++ b/lib/parsers/outdated_parser.rb @@ -71,9 +71,9 @@ def split_row(line) def build_row(cells) name, current, latest_or_error = cells[0], cells[1], cells[2] if VERSION_SHAPE_RE.match?(latest_or_error) - OutdatedPackage.new(name: name, current: current, latest: latest_or_error, error: nil) + OutdatedPackage.new(name:, current:, latest: latest_or_error, error: nil) else - OutdatedPackage.new(name: name, current: current, latest: nil, error: latest_or_error) + OutdatedPackage.new(name:, current:, latest: nil, error: latest_or_error) end end end diff --git a/lib/planner.rb b/lib/planner.rb index 39019a0..48619b7 100644 --- a/lib/planner.rb +++ b/lib/planner.rb @@ -190,7 +190,7 @@ def build_non_security_specs(bumps) def build_grouped_spec(kind, bumps) PRSpec.new( - kind: kind, + kind:, packages: bumps, branch: "#{@config.branch_prefix}/#{kind}", title: grouped_title(kind, bumps), @@ -200,7 +200,7 @@ def build_grouped_spec(kind, bumps) def build_individual_spec(kind, bump) PRSpec.new( - kind: kind, + kind:, packages: [bump], branch: "#{@config.branch_prefix}/#{kind}-#{sanitize(bump.name)}", title: with_prefix("bump #{bump.name} #{bump.from} → #{bump.to}"), @@ -229,7 +229,7 @@ def with_prefix(message) def metadata_for(kind, bumps) { tool: "importmap-update", - kind: kind, + kind:, packages: bumps.map { |b| entry = {name: b.name, from: b.from, to: b.to, semver_kind: b.semver_kind} entry[:severity] = b.advisory[:severity] if b.advisory diff --git a/test/executor_test.rb b/test/executor_test.rb index fca9e38..01b4efd 100644 --- a/test/executor_test.rb +++ b/test/executor_test.rb @@ -30,19 +30,19 @@ def ensure_labels(labels) end def create_pr(branch:, base:, title:, body:, labels: []) - @created << {branch: branch, base: base, title: title, body: body, labels: labels} + @created << {branch:, base:, title:, body:, labels:} n = @next_pr_number @next_pr_number += 1 n end def update_pr(number:, title:, body:) - @updated << {number: number, title: title, body: body} + @updated << {number:, title:, body:} nil end def close_pr(number:, comment: nil) - @closed << {number: number, comment: comment} + @closed << {number:, comment:} nil end end @@ -60,7 +60,7 @@ def initialize end def checkout_fresh_branch(branch:, base:) - @checkouts << {branch: branch, base: base} + @checkouts << {branch:, base:} nil end @@ -70,7 +70,7 @@ def commit_changes(message:) end def push(branch:, force: false) - @pushes << {branch: branch, force: force} + @pushes << {branch:, force:} nil end end @@ -78,22 +78,22 @@ def push(branch:, force: false) # ---- builders mirroring the planner's output ---- def bump(name, from, to, kind: :patch, severity: nil) - advisory = severity ? {severity: severity} : nil - Planner::PackageBump.new(name: name, from: from, to: to, semver_kind: kind, advisory: advisory) + advisory = severity ? {severity:} : nil + Planner::PackageBump.new(name:, from:, to:, semver_kind: kind, advisory:) end def spec(branch:, packages:, kind: :patch, title: "spec title") Planner::PRSpec.new( - kind: kind, packages: packages, branch: branch, title: title, + kind:, packages:, branch:, title:, metadata: { - tool: "importmap-update", kind: kind, + tool: "importmap-update", kind:, packages: packages.map { |p| {name: p.name, from: p.from, to: p.to, semver_kind: p.semver_kind} } } ) end def existing_pr(number:, branch:) - Reconciler::ExistingPR.new(number: number, branch: branch, body: "", title: "old") + Reconciler::ExistingPR.new(number:, branch:, body: "", title: "old") end def setup @@ -108,7 +108,7 @@ def make_executor(dry_run: false) base_branch: "main", commit_message_prefix: "", labels: %w[dependencies], - dry_run: dry_run, + dry_run:, # Override the body renderer so tests don't depend on the exact body string. body_renderer: ->(s) { "body for #{s.branch}" } ) diff --git a/test/fixture_runner.rb b/test/fixture_runner.rb index c441c96..bfbb628 100644 --- a/test/fixture_runner.rb +++ b/test/fixture_runner.rb @@ -17,8 +17,8 @@ def initialize(fixtures = []) def add(pattern:, stdout: "", stderr: "", exit_code: 0) @fixtures << Fixture.new( - pattern: pattern, - result: Result.new(stdout: stdout, stderr: stderr, exit_code: exit_code) + pattern:, + result: Result.new(stdout:, stderr:, exit_code:) ) end diff --git a/test/planner_test.rb b/test/planner_test.rb index 7345e51..8bfcb3f 100644 --- a/test/planner_test.rb +++ b/test/planner_test.rb @@ -15,15 +15,15 @@ class PlannerTest < Minitest::Test # ---- helpers ---- def outdated(name, from, to, error: nil) - Outdated.new(name: name, current: from, latest: to, error: error) + Outdated.new(name:, current: from, latest: to, error:) end def vuln(name, severity, vulnerable: "<#{name}", advisory: "Vulnerability in #{name}") - Vuln.new(name: name, severity: severity, vulnerable_versions: vulnerable, advisory: advisory) + Vuln.new(name:, severity:, vulnerable_versions: vulnerable, advisory:) end def plan_with(outdated:, vulnerabilities: [], config: Config.default) - Planner.new(outdated: outdated, vulnerabilities: vulnerabilities, config: config).call + Planner.new(outdated:, vulnerabilities:, config:).call end # ---- happy paths: grouping ---- @@ -178,7 +178,7 @@ def test_individual_patch_strategy_emits_one_pr_per_package outdated("lodash", "4.17.20", "4.17.21"), outdated("axios", "1.7.0", "1.7.1") ], - config: config + config: ) assert_equal 2, plan.pr_specs.size assert(plan.pr_specs.all? { |s| s.kind == :patch }) @@ -198,7 +198,7 @@ def test_grouped_major_strategy_combines_into_single_pr outdated("react", "18.2.0", "19.0.0"), outdated("vue", "2.7.0", "3.0.0") ], - config: config + config: ) assert_equal 1, plan.pr_specs.size assert_equal :major, plan.pr_specs.first.kind @@ -276,7 +276,7 @@ def test_open_pull_requests_limit_throttles_non_security_prs outdated("b", "1.0.0", "2.0.0"), outdated("c", "1.0.0", "2.0.0") ], - config: config + config: ) assert_equal 2, plan.pr_specs.size assert(plan.warnings.any? { |w| w.include?("Dropped 1") }) @@ -296,7 +296,7 @@ def test_security_prs_are_never_throttled_even_above_the_limit vuln("vulnB", "high"), vuln("vulnC", "high") ], - config: config + config: ) security = plan.pr_specs.select(&:security?) assert_equal 3, security.size, "security PRs should not be throttled" @@ -312,7 +312,7 @@ def test_budget_keeps_higher_priority_prs_when_truncating outdated("patch-pkg", "1.0.0", "1.0.1"), # patch (grouped) outdated("major-pkg", "1.0.0", "2.0.0") # major (individual) ], - config: config + config: ) # Major should win over patch when only 1 slot is available. assert_equal 1, plan.pr_specs.size diff --git a/test/reconciler_test.rb b/test/reconciler_test.rb index 9e5a42b..fd0b1f3 100644 --- a/test/reconciler_test.rb +++ b/test/reconciler_test.rb @@ -16,19 +16,19 @@ class ReconcilerTest < Minitest::Test # ---- builder helpers ---- def package_bump(name, from, to, kind: :patch, severity: nil) - advisory = severity ? {severity: severity, vulnerable_versions: "<#{to}", description: "..."} : nil - Planner::PackageBump.new(name: name, from: from, to: to, semver_kind: kind, advisory: advisory) + advisory = severity ? {severity:, vulnerable_versions: "<#{to}", description: "..."} : nil + Planner::PackageBump.new(name:, from:, to:, semver_kind: kind, advisory:) end def spec(branch:, packages:, kind: :patch, title: "spec title") Planner::PRSpec.new( - kind: kind, - packages: packages, - branch: branch, - title: title, + kind:, + packages:, + branch:, + title:, metadata: { tool: "importmap-update", - kind: kind, + kind:, packages: packages.map { |p| entry = {name: p.name, from: p.from, to: p.to, semver_kind: p.semver_kind} entry[:severity] = p.advisory[:severity] if p.advisory @@ -48,14 +48,14 @@ def existing_pr(number:, branch:, packages:, title: "old title") metadata = { tool: "importmap-update", kind: :patch, - packages: packages.map { |(name, from, to)| {name: name, from: from, to: to, semver_kind: :patch} } + packages: packages.map { |(name, from, to)| {name:, from:, to:, semver_kind: :patch} } } body = "Description.\n\n" + Metadata.render(metadata) - ExistingPR.new(number: number, branch: branch, body: body, title: title) + ExistingPR.new(number:, branch:, body:, title:) end def reconcile(plan_specs:, existing_prs: []) - Reconciler.new(plan: plan_with(plan_specs), existing_prs: existing_prs).call + Reconciler.new(plan: plan_with(plan_specs), existing_prs:).call end # ---- :open ---- @@ -102,7 +102,7 @@ def test_noop_when_package_order_differs_but_set_is_equal package_bump("lodash", "4.17.20", "4.17.21"), package_bump("axios", "1.7.0", "1.7.1") ] - s = spec(branch: "importmap-updates/patch", packages: packages) + s = spec(branch: "importmap-updates/patch", packages:) existing = existing_pr( number: 7, branch: "importmap-updates/patch", @@ -226,7 +226,7 @@ def test_treats_pr_with_wrong_tool_marker_as_foreign - { name: lodash, from: 4.17.20, to: 4.17.21 } --> BODY - foreign = ExistingPR.new(number: 60, branch: "importmap-updates/patch", body: body, title: "...") + foreign = ExistingPR.new(number: 60, branch: "importmap-updates/patch", body:, title: "...") result = reconcile(plan_specs: [], existing_prs: [foreign]) assert_empty result.actions @@ -334,7 +334,7 @@ def test_treats_pr_with_unparseable_metadata_block_as_foreign : : --> BODY - pr = ExistingPR.new(number: 70, branch: "importmap-updates/patch", body: body, title: "...") + pr = ExistingPR.new(number: 70, branch: "importmap-updates/patch", body:, title: "...") result = reconcile(plan_specs: [], existing_prs: [pr]) # Better to leave it alone than to misinterpret and close it. From 22609b1787c3d5386b6bff174d7d4254a2819b37 Mon Sep 17 00:00:00 2001 From: Neil Carvalho Date: Tue, 26 May 2026 08:58:19 -0300 Subject: [PATCH 3/3] Move conditional up --- lib/parsers/outdated_parser.rb | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/lib/parsers/outdated_parser.rb b/lib/parsers/outdated_parser.rb index e6de8e8..c5975e6 100644 --- a/lib/parsers/outdated_parser.rb +++ b/lib/parsers/outdated_parser.rb @@ -69,12 +69,11 @@ def split_row(line) end def build_row(cells) - name, current, latest_or_error = cells[0], cells[1], cells[2] - if VERSION_SHAPE_RE.match?(latest_or_error) - OutdatedPackage.new(name:, current:, latest: latest_or_error, error: nil) - else - OutdatedPackage.new(name:, current:, latest: nil, error: latest_or_error) - end + name, current, latest_or_error = cells + latest = latest_or_error if VERSION_SHAPE_RE.match?(latest_or_error) + error = latest_or_error unless VERSION_SHAPE_RE.match?(latest_or_error) + + OutdatedPackage.new(name:, current:, latest:, error:) end end end