Phase 7: conditional formatting - #129
Merged
Merged
Conversation
xl_cond_cell() covers the rules that pair a condition with a format -- comparisons, text matches, time periods, above/below average, top/bottom N, duplicates, uniques, blanks, errors and arbitrary formulas -- reaching worksheet_conditional_format_range(). xl_sheet(conditional =) takes one or a list of them through the Phase 0 overlay stage. The 34 criteria are partitioned by rule type and libxlsxwriter checks none of the pairing, so a text criteria on a cell rule produces a file Excel accepts and then silently ignores. .CONDITIONAL_CRITERIA_FOR maps each type to what it accepts, and a mismatch is an error naming both the criteria and the type and listing what would be valid there. Where the two are redundant the type is inferred from the criteria instead. The format is an ordinary xl_format, so it goes through the existing registry unchanged -- but conditional formats are emitted as differential formats. That was worth verifying rather than assuming: a rule's fill lands in <dxfs> in styles.xml and the rule references it by dxfId, which a test now pins. The API consistency gate added in the previous review caught this before it shipped: xl_cond_cell() had criteria before type while xl_validation() has type before criteria, and the two share six arguments. Reordered to match, since in both functions the type is what determines which criteria are legal. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
xl_cond_scale() writes two- and three-colour scales, xl_cond_bar() in-cell data bars with control over the negative and border colours, axis and direction, and xl_cond_icons() one of Excel's seventeen built-in icon sets. Each is a separate constructor because the clusters share almost no fields; all four flow through one overlay kind. Corrects the conditional type table. Three of libxlsxwriter's type constants -- 2_COLOR_SCALE, 3_COLOR_SCALE and DATA_BAR -- carry no TYPE_ in their names, so an earlier reading of the header missed them and put icon_sets at 14 rather than 17. That is the index of a two-colour scale, so every icon set would have silently written a scale instead. The commit before this one only used indices 0 to 13 and was unaffected. The corrected mapping is pinned by enumerating all seventeen styles against the names Excel stores, which is the test that would have caught the original error: 5_quarters must give 5Quarters, 4_ratings must give 4Rating, and so on. A further test asserts the map still has seventeen entries, so a style added by a libxlsxwriter update fails here rather than being quietly unreachable. Icon sets embed nothing: Excel draws them itself, and a test asserts no xl/media part appears. 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.
Part of the libxlsxwriter API coverage roadmap in #112.
Covers
worksheet_conditional_format_cell()and_range()through fourconstructors, since the struct's ~30 fields fall into clusters that share almost
nothing:
Two silent-wrong-output traps closed
The 34 criteria are partitioned by rule type, and libxlsxwriter checks none of
the pairing. A text criteria on a cell rule produces a file Excel accepts and
then silently ignores.
.CONDITIONAL_CRITERIA_FORmaps each type to what itaccepts; a mismatch errors naming both and listing what would be valid there.
Where the two are redundant, the type is inferred from the criteria.
Three of libxlsxwriter's type constants carry no
TYPE_in their names —LXW_CONDITIONAL_2_COLOR_SCALE,_3_COLOR_SCALE,_DATA_BAR. My first readingof the header missed them and put
icon_setsat 14 instead of 17. Fourteen isthe index of a two-colour scale, so every icon set would have silently written
a colour scale. Caught by going back to the header rather than trusting the
earlier extraction.
The fix is pinned by enumerating all seventeen styles against the names Excel
stores —
5_quarters→5Quarters,4_ratings→4Rating— which is exactlythe test that would have caught the original error. A further test asserts the
map still has seventeen entries, so a style added by a libxlsxwriter update
fails loudly rather than becoming quietly unreachable.
The consistency gate earned its keep
xl_cond_cell()was written withcriteriabeforetype, whilexl_validation()hastypebeforecriteria— six shared arguments inconflicting order. The gate added in #127 failed the build before this shipped.
Reordered to match, on the principle that in both functions the type is what
determines which criteria are legal.
Verified, not assumed
<dxfs>: conditional formats use differential formats rather thancellXfs, so I could not assume the shared format registry would landcorrectly. It does — a rule's fill appears in
<dxfs>and the rule referencesit by
dxfId. Pinned by a test.xl/mediapart appears, which is why they did not need to wait for Phase 10.optimizeguard in either function; a testasserts row streaming stays on.
Verification
devtools::check(): 0 errors, 0 warnings, 0 notescellIswith eachoperator,
colorScalewith 2 and 3 stops,dataBar,iconSetrule carries its own meaning;
betweenmissing a bound; scales with 1 or 4colours; mismatched
values/rule_typeslengths; all 17 icon styles