refactor: collapse opts merging to single authoritative path#13
Merged
Conversation
Remove the DEEP_MERGE strategy and drop `opts` from `merge_specs` entirely. Function-form opts could never be resolved at merge time (the `plugin` arg doesn't exist yet), so `resolve_opts` already had to run at load time — the merge-time DEEP_MERGE pass just produced an orphaned value that no runtime code read for its value, only for its nil-ness. This left two parallel opts pipelines and made `merged_spec.opts` subtly wrong whenever function-form and table-form were mixed. Now: - `merge_specs` skips `opts`. `merged_spec.opts` is always nil. - `resolve_all` computes `entry.has_opts` once, a boolean summary used by the three existence checks in `plugin_loader` and `startup`. - `run_config` always calls `resolve_opts(sorted_specs, plugin)` — the single-spec fast path is gone, which has the side benefit of always handing `config()` a fresh table instead of a live reference to the spec's underlying opts. - `merge_spec_array` now always folds from an empty base, so the returned table is always freshly allocated and never aliases an input spec. Tests updated to reflect the new contract: - `opts_test.lua`: assert `merged_spec.opts == nil` and check `entry.has_opts` / `sorted_specs[1].opts` instead. - `merge_test.lua`: the old "merge_specs deep merges opts" test now asserts merge_specs skips opts and that `resolve_opts` is the deep-merge path. - `is_single_spec_test.lua`: same treatment.
f674275 to
8cc4bb2
Compare
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
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.
Remove the DEEP_MERGE strategy and drop
optsfrommerge_specsentirely. Function-form opts could never be resolved at merge time (thepluginarg doesn't exist yet), soresolve_optsalready had to run at load time — the merge-time DEEP_MERGE pass just produced an orphaned value that no runtime code read for its value, only for its nil-ness. This left two parallel opts pipelines and mademerged_spec.optssubtly wrong whenever function-form and table-form were mixed.Now:
merge_specsskipsopts.merged_spec.optsis always nil.resolve_allcomputesentry.has_optsonce, a boolean summary used by the three existence checks inplugin_loaderandstartup.run_configalways callsresolve_opts(sorted_specs, plugin)— the single-spec fast path is gone, which has the side benefit of always handingconfig()a fresh table instead of a live reference to the spec's underlying opts.merge_spec_arraynow always folds from an empty base, so the returned table is always freshly allocated and never aliases an input spec.Tests updated to reflect the new contract:
opts_test.lua: assertmerged_spec.opts == niland checkentry.has_opts/sorted_specs[1].optsinstead.merge_test.lua: the old "merge_specs deep merges opts" test now asserts merge_specs skips opts and thatresolve_optsis the deep-merge path.is_single_spec_test.lua: same treatment.