Skip to content

Treat a blank at the start of a value block as layout, not as value - #205

Merged
headmeister merged 2 commits into
isi-nmr:masterfrom
gdevenyi:fix/jcampdx-leading-blank
Aug 10, 2026
Merged

Treat a blank at the start of a value block as layout, not as value#205
headmeister merged 2 commits into
isi-nmr:masterfrom
gdevenyi:fix/jcampdx-leading-blank

Conversation

@gdevenyi

@gdevenyi gdevenyi commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

Fixes #193.

_normalize_line_breaks only rstrip()ed, so the blank ParaVision writes after an opening ( stayed glued to element 0. parse_value tests the string shape with val_str.startswith("<"), which is then False, so the only branch that calls unquote_string is skipped and the token is returned verbatim:

parse_value('( <A>, <B> )')  ->  ['<A>', 'B']      # asymmetric
parse_value('( 1, 2 )')      ->  ['1', 2]          # leading int becomes a str

On real files:

##$SUBJECT_study_adj_config=( <MRI_Default> , <MRI Default> )
  ->  ['<MRI_Default>', 'MRI Default']

so value[0] == 'MRI_Default' was silently False, and the write path re-quoted the kept delimiters into <<MRI_Default>>. 613 parameter occurrences across the corpus were affected — CONFIG_SCAN_operation_mode (the spec's own worked example for this shape), AdjConfigurationMode, CONFIG_SCAN_RxCoilsNames, CONFIG_SCAN_TxCoilsNames, SHIM_status_check_sum_components, SUBJECT_study_adj_config, PVM_StartupShimList.

Change

rstrip()strip(), at the one shared point. That repairs the string case and the numeric case together. A value block begins after ##$name= or after a ( / , delimiter and ends where the next ## record begins, so blanks at either end are layout. Blanks inside the value are untouched — ParaVision's wrap puts the space before the newline it inserts, so joining the lines still reassembles the string.

After the fix, on real files:

SUBJECT_study_adj_config   -> ['MRI_Default', 'MRI Default']
PVM_StartupShimList        -> ['S0', 'Spherical Shim.']
CONFIG_SCAN_RxCoilsNames   -> ['1', 'Chan1[1H]: RF ARR 400 1H M.BR. 2x2 RO AD']

Tests

  • test_a_blank_after_the_opening_paren_is_layout_not_value — the three shapes plus a subject file written inline. Fails on master.
  • test_blanks_inside_a_wrapped_string_are_kept — a wrap inside a <...> string, guarding the blanks the change must not touch.

Suite: 2140 passed, 12 skipped (unchanged). Corpus load test unchanged at 3202/3478 with an identical failure breakdown.

@gdevenyi
gdevenyi force-pushed the fix/jcampdx-leading-blank branch from 1634d1f to 351cc6f Compare August 8, 2026 19:50
gdevenyi and others added 2 commits August 8, 2026 16:33
`ruff format --check` reported twelve files as unformatted, so any change
touching one of them dragged unrelated reflow into its diff. Run the
formatter once, on its own, so subsequent changes show only what they
actually change.

Formatting only: no behaviour changes, and `ruff check` is clean before and
after. The suite is unchanged at 2140 passed, 12 skipped.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017SNrm3jFhpTPShP8xGkePC
_normalize_line_breaks only rstrip()ed, so a blank ParaVision writes after
an opening `(` stayed glued to the first element of the value.

parse_value tests the string shape with val_str.startswith("<"), which is
then False, so the only branch that calls unquote_string is skipped and the
lone remaining token is returned verbatim -- brackets included. Later
elements have no leading blank and are unquoted, so the PV6/PV360
(name, display-name) enum tuple came back asymmetric:

    ##$SUBJECT_study_adj_config=( <MRI_Default> , <MRI Default> )
    -> ['<MRI_Default>', 'MRI Default']

Comparing such a value against its documented symbol silently never
matched, and writing it back re-quoted the delimiters it had kept
(<<MRI_Default>>). The same missing lstrip mistyped a leading number:
parse_value('( 1, 2 )') gave ['1', 2].

Strip both ends of the block instead. A value block begins after `##$name=`
or after a `(` / `, ` delimiter and ends where the next ## record begins, so
blanks at either end are layout. Blanks *inside* the value are untouched:
ParaVision's wrap puts the space before the newline it inserts, so joining
the lines still reassembles the string.

613 parameter occurrences across the corpus were affected --
CONFIG_SCAN_operation_mode, AdjConfigurationMode, CONFIG_SCAN_RxCoilsNames,
CONFIG_SCAN_TxCoilsNames, SHIM_status_check_sum_components,
SUBJECT_study_adj_config and PVM_StartupShimList.

Closes isi-nmr#193

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017SNrm3jFhpTPShP8xGkePC
@gdevenyi
gdevenyi force-pushed the fix/jcampdx-leading-blank branch from 351cc6f to bb7d4a9 Compare August 8, 2026 20:50
@gdevenyi

gdevenyi commented Aug 8, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto #213 (ruff format), so this branch carries that commit until #213 merges — merge #213 first and this diff reduces to its own change. Verified after the rebase: ruff check and ruff format --check clean, full suite green.

Integration check across all eleven conformance PRs merged together: 2164 passed, 12 skipped; corpus load test 3202 → 3207 of 3478 with zero newly broken datasets.

@headmeister
headmeister merged commit 43a99a3 into isi-nmr:master Aug 10, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

parse_value keeps the <...> delimiters on a value's first element when a blank follows the opening paren (spec 2.2)

2 participants