fix(figure): validate data_prefix instead of emitting unparseable GLE - #13
Merged
Conversation
`Figure(data_prefix=...)` was used verbatim to build sidecar filenames
without any checking, so a prefix GLE cannot parse only failed much later,
at compile time, with a message pointing at generated GLE rather than at
the bad value:
glp.figure(data_prefix="mk+white")
# >> Error: left hand side contains unquoted string
# on `data mk+white_0.dat d1=c1,c2`
`Figure.__init__` now raises ValueError (naming the offending characters
and their positions) so the failure surfaces in Python at the point the
value was supplied.
Validate rather than sanitize. The per-series `data_name=` path runs
through `_sanitize_data_stem`, but the two arguments differ in kind:
`data_name` takes a free-form *label* ("Observed Signal") whose mapping to
a filename is an implementation detail, whereas `data_prefix` exists
precisely so callers can *predict* sidecar names in batch pipelines.
Silently rewriting `mk+white` to `mk_white` would trade a GLE parse error
for a missing file even further downstream, in the caller's own tooling.
`_sanitize_data_stem` also lowercases, so reusing it would have quietly
turned the documented `data_prefix='experimentA'` into `experimenta_0.dat`
— a regression in documented behaviour.
The rejected set is drawn from GLE 4.3.10, not assumption: compiling a
`data <file>` statement for every printable ASCII punctuation character
shows only `!`, `"`, `+` and whitespace break its unquoted-filename
tokenizer. `.`, `-`, `#`, `,`, `=`, `*` and non-ASCII all parse fine and
stay allowed. `/` and `\` are additionally rejected on every platform as
path separators, since a prefix names a filename stem, not a path.
`Figure.from_dict` restores the prefix verbatim, bypassing validation:
that is recorded state, not user input. The recognizer derives a prefix
from filenames already in a parsed `.gle`, and a quoted
`data "my file_0.dat"` legitimately yields `my file`; validating there
would make such a figure impossible to reload and would break GUI
undo/redo, which restores every snapshot through `from_dict`.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
What
Figure(data_prefix=...)was used verbatim to build sidecar.datfilenames with no checking, so a prefix GLE cannot parse failed only at compile time, pointing at generated GLE rather than at the bad value:Figure.__init__now raisesValueError, naming the offending characters and their positions:Why validate rather than sanitize
The per-series
data_name=path already runs through_sanitize_data_stem, but the two arguments differ in kind:data_nametakes a free-form label ('Observed Signal') whose mapping to a filename is an implementation detail — sanitizing is the point.data_prefixexists precisely so callers can predict sidecar names in batch pipelines. Silently rewritingmk+white→mk_whitewould trade a GLE parse error for a missing file even further downstream, in the caller's own tooling._sanitize_data_stemalso lowercases, so reusing it would have quietly turned the documenteddata_prefix='experimentA'intoexperimenta_0.dat— a regression in documented behaviour (docs/source/getting_started.rst).Where the rejected set comes from
Empirical, not assumed. I compiled a
data <file>statement against GLE 4.3.10 for every printable ASCII punctuation character, with a missing-file control to confirm the probe actually detected resolution failures:!,",+, and whitespace. Nothing else..-_#$%&()[]{},;:=@~^*?<>|`'and non-ASCII./and\, on every platform, as path separators — a prefix names a filename stem, not a path.Empty/whitespace-only prefixes are rejected too (pass
Nonefor defaultdata_N.datnaming); a non-strraisesTypeError.Reviewer notes
Figure.from_dictdeliberately bypasses validation. That is recorded state, not user input: the recognizer derives a prefix from filenames already present in a parsed.gle, and a quoteddata "my file_0.dat"legitimately yields the prefixmy file. Validating on restore would make such a figure impossible to reload and would break GUI undo/redo, which routes every snapshot throughfrom_dict. There's a test pinning this.Two adjacent issues left out of scope, flagged rather than fixed:
fig.data_prefix = "mk+white"after construction is still unvalidated. A property setter would catch it but would also fire on the recognizer and GUI-preview paths that assign directly, so widening the change didn't seem right here..glewhose data filename was quoted can round-trip to a prefix that gleplot's writer then re-emits unquoted — a real pre-existing bug, but a separate one.Testing
TestDataPrefixValidationintests/unit/test_plotting.py(10 cases), placed alongside the existingdata_namenaming tests.black-clean; formatting was applied only to the new regions to avoid repo-wide quote churn.Docs
docs/guides/CONFIGURATION.md— new "Allowed characters" subsection under the existingdata_prefixsection, including the contrast withdata_name.docs/source/getting_started.rst— short constraints note on theexperimentAexample.data_prefixat all (only a genericadvanced/directory listing) andDOCUMENTATION_INDEX.mdonly indexes the example filename, so neither needed a change.🤖 Generated with Claude Code