Generate no lookup conf for an unnamed variable - #250
Merged
Conversation
`name` is both the output variable name and the key into the ad's frozen ad_attributions row, so a lookup conf without one reads nothing off the row while presenting itself in the form as configured — a filled-in row that can only ever yield nothing. The guard was `variables.length === 0`, which one unnamed variable passes: its length is 1. Filtering the names is what actually expresses the rule, and a source whose variables are all unnamed falls back to the blank row it would have shown anyway. Also pins two ref_mode properties that held but were untested: a conf stating an explicit mode keeps it through an unrelated edit, and a mode matching what was loaded does not warn. Found comparing the shipped implementation against feature/ref-mode-dashboard-ux (PR #246), which built the same design independently and carries a test for this case. Both new tests fail without the fix. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
✅ Deploy Preview for vlab-dashboard ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
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.
nameis both the output variable name and the key into the ad's frozenad_attributionsrow, so a lookup conf without one reads nothing off the rowwhile presenting itself in the form as configured — a filled-in row that can
only ever yield nothing.
The guard was
variables.length === 0, which one unnamed variable passes: itslength is 1.
Also pins two
ref_modeproperties that held but were untested: a conf statingan explicit mode keeps it through an unrelated edit, and a mode matching what
was loaded does not warn.
Provenance. Found by comparing what shipped in #247 against
feature/ref-mode-dashboard-ux(#246), which built the same designindependently and carries
generateLookupConfs.test.tscovering this case. AnAST-level comparison showed adopt and swoosh are behaviourally identical between
the two implementations —
events.gobyte-identical, and the only differencesin
study_conf.py/marketing.py/csv_export.pyare one variable rename,one tightened optional parameter, and one error-message wording. This was the
one real gap.
Both new
generateLookupConfstests fail without the fix.Dashboard only — Netlify deploys from main on merge.
🤖 Generated with Claude Code