Skip to content

Add locale decimal condition examples#523

Merged
PrzemyslawKlys merged 5 commits intomasterfrom
codex/locale-decimal-examples
Mar 6, 2026
Merged

Add locale decimal condition examples#523
PrzemyslawKlys merged 5 commits intomasterfrom
codex/locale-decimal-examples

Conversation

@PrzemyslawKlys
Copy link
Member

Summary

  • add a PSWriteHTML example covering HTML and JavaScript table stores with dot and comma decimals
  • add a focused regression test for numeric threshold serialization and the PowerShell 0,2 array pitfall
  • document the PowerShell decimal-value guidance in New-TableCondition help

Verification

  • Invoke-Pester -Path Tests\New-TableCondition.Tests.ps1 -Output Detailed
  • generated Examples/Example-TableColumnHighlight/Example-LocaleDecimals.html
  • browser-verified that 0.17 and 0,17 render green while 0.25 and 0,25 render red in all four tables

Copilot AI review requested due to automatic review settings March 6, 2026 22:07
@claude
Copy link

claude bot commented Mar 6, 2026

PR Review for Add locale decimal condition examples. See detailed notes below.

DOCUMENTATION - PASS. The improved parameter description and examples are a clear improvement. The explanation of the 0,2 PowerShell parsing trap is exactly what users need.

EXAMPLE SCRIPT - PASS. The four-scenario matrix covering HTML/JS store with dot/comma decimals is a good teaching tool. Two minor notes: (1) htmlDot uses string values like 0.17 rather than numerics - intentional for HTML-store tables but worth a comment. (2) jsComma uses strings with -DataStore JavaScript - also intentional but a brief comment would help readers understand the distinction from jsDot.

GENERATED HTML - WARNING. The .html file is checked in with a hardcoded meta-revised timestamp (2026-03-06 15:54:04). The timestamp will be stale after merging, and future changes to the .ps1 require manually re-generating the .html. Consider whether generated output needs to live in source control.

TESTS

Test 1 (Serializes dot and quoted comma thresholds) - PASS. The three Should -Match assertions confirm correct JSON serialization for both decimal formats.

Test 2 (Receives bare comma decimals as an array) - WARNING with four concerns:
(1) Mostly tests PowerShells own parser, not PSWriteHTML - assertions on bareCommaValue verify PowerShell behavior and pass regardless of any PSWriteHTML change.
(2) Accessing conditionWithBareComma.Output.Value is fragile - couples the test to internal implementation details.
(3) The generated TemporaryConditionArray.html is never read or validated.
(4) No assertion on library behavior - the test does not verify whether PSWriteHTML warns or handles the unexpected array in any particular way.

Suggestion for test 2: either remove it (docs already cover the pitfall) or replace with a -WarningVariable assertion verifying library behavior on array input.

SUMMARY: Documentation PASS, Example PASS, Generated HTML WARNING (stale timestamp), Test 1 PASS, Test 2 WARNING (tests PS internals). The core change is valuable and ready to merge. Test 2 is worth revisiting.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds documentation, an example, and a regression test to clarify/validate how New-TableCondition handles numeric thresholds when values use dot vs comma decimal formatting in PowerShell + DataTables.

Changes:

  • Added Pester coverage to verify threshold serialization for dot decimals (numeric) and comma decimals (quoted string), plus a regression test for PowerShell’s 0,2 array-parsing pitfall.
  • Updated New-TableCondition comment-based help with explicit guidance and concrete examples for dot vs comma decimal usage.
  • Added a new example script demonstrating the behavior across HTML and JavaScript table data stores (and also checked in the generated HTML output).

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File Description
Tests/New-TableCondition.Tests.ps1 Adds regression tests around locale decimal handling and serialization.
Public/New-TableCondition.ps1 Expands help text and examples documenting dot vs comma decimal guidance.
Examples/Example-TableColumnHighlight/Example-LocaleDecimals.ps1 New example script demonstrating locale decimal behavior across data stores.
Examples/Example-TableColumnHighlight/Example-LocaleDecimals.html Generated HTML output for the new example (likely should not be committed).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

$Content = Get-Content -Path $FilePath -Raw
$Content | Should -Match '"columnHighlighter"'
$Content | Should -Match '"value": "0,2"'
$Content | Should -Match '"value": 0.2'
Copy link

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The assertion Should -Match '"value": 0.2' uses regex matching where . is a wildcard, so it would also match values like 0,2 or 0x2. Escape the dot (e.g., 0\.2) or use a more precise pattern (including optional whitespace / word boundary) to ensure the test is actually validating dot-decimal numeric serialization.

Suggested change
$Content | Should -Match '"value": 0.2'
$Content | Should -Match '"value":\s*0\.2\b'

Copilot uses AI. Check for mistakes.
$bareCommaValue = 0,2

New-HTML {
$script:conditionWithBareComma = New-TableCondition -Name 'Value' -ComparisonType number -Operator le -Value $bareCommaValue
Copy link

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inside the New-HTML { ... } block, $script:conditionWithBareComma = New-TableCondition ... will emit the assigned object because assignments return the assigned value in PowerShell. That can cause New-HTML to include an unintended stringified object in the generated HTML. Suppress output by assigning to $null (e.g., $null = ($script:conditionWithBareComma = ...)) or otherwise preventing the condition object from being written to the pipeline.

Suggested change
$script:conditionWithBareComma = New-TableCondition -Name 'Value' -ComparisonType number -Operator le -Value $bareCommaValue
$null = ($script:conditionWithBareComma = New-TableCondition -Name 'Value' -ComparisonType number -Operator le -Value $bareCommaValue)

Copilot uses AI. Check for mistakes.
Comment on lines +1 to +5
<!DOCTYPE html>
<html lang="en"><head><meta charset="utf-8"><meta name="viewport" content="width=device-width, initial-scale=1"><meta name="revised" content="2026-03-06 15:54:04"><title>PSWriteHTML decimal number conditions</title><link type="text/css" href="https://fonts.googleapis.com/css2?family=Roboto+Condensed&display=swap" rel="stylesheet preload prefetch" as="style"/><script type="text/javascript" src="https://cdn.jsdelivr.net/npm/jquery@3.7.0/dist/jquery.min.js" comment="jquery"></script><script type="text/javascript" src="https://cdn.jsdelivr.net/npm/moment@2.29.4/moment.min.js"></script><link type="text/css" href="https://cdn.datatables.net/1.13.5/css/jquery.dataTables.min.css" rel="stylesheet preload prefetch" as="style"/><link type="text/css" href="https://cdn.datatables.net/select/1.7.0/css/select.dataTables.min.css" rel="stylesheet preload prefetch" as="style"/><script type="text/javascript" src="https://cdn.datatables.net/1.13.5/js/jquery.dataTables.min.js"></script><script type="text/javascript" src="https://cdn.datatables.net/select/1.7.0/js/dataTables.select.min.js"></script><script type="text/javascript" src="https://cdn.datatables.net/plug-ins/1.13.5/sorting/datetime-moment.js"></script><link type="text/css" href="https://cdn.jsdelivr.net/npm/enlighterjs@3.4.0/dist/enlighterjs.min.css" rel="stylesheet preload prefetch" as="style"/><script type="text/javascript" src="https://cdn.jsdelivr.net/npm/enlighterjs@3.4.0/dist/enlighterjs.min.js"></script><link type="text/css" href="https://cdn.datatables.net/buttons/2.4.0/css/buttons.dataTables.min.css" rel="stylesheet preload prefetch" as="style"/><script type="text/javascript" src="https://cdn.datatables.net/buttons/2.4.0/js/dataTables.buttons.min.js"></script><script type="text/javascript" src="https://cdnjs.cloudflare.com/ajax/libs/jszip/3.10.1/jszip.min.js"></script><script type="text/javascript" src="https://cdn.datatables.net/buttons/2.4.0/js/buttons.html5.min.js"></script><script type="text/javascript" src="https://cdnjs.cloudflare.com/ajax/libs/pdfmake/0.2.7/pdfmake.min.js"></script><script type="text/javascript" src="https://cdnjs.cloudflare.com/ajax/libs/pdfmake/0.2.7/vfs_fonts.js"></script><script type="text/javascript" src="https://cdn.jsdelivr.net/npm/@evotecit/htmlextensions@0.1.12/dist/datatables.columnHighlighter.js"></script><link type="text/css" href="https://cdn.datatables.net/responsive/2.5.0/css/responsive.dataTables.min.css" rel="stylesheet preload prefetch" as="style"/><script type="text/javascript" src="https://cdn.datatables.net/responsive/2.5.0/js/dataTables.responsive.min.js"></script><style type="text/css">body { font-family: 'Roboto Condensed', sans-serif; font-size: 8pt; margin: 0px;}input { font-size: 8pt;}.main-section { margin-top: 0px;}</style><style type="text/css">td:first-child { white-space: nowrap;}div.dataTables_wrapper { margin: 5px;}tfoot input { padding: -3px; box-sizing: border-box; width: 100%;}thead input { padding: -3px; box-sizing: border-box; width: 100%;}td::before, td.sorting_1::before { color: #007bff !important;}</style><noscript><style type="text/css">table {
border-collapse: collapse;
box-sizing: border-box;
width: 100%;
Copy link

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file appears to be a generated artifact from running the example script (includes a timestamped meta name="revised", random table IDs, and the full bundled HTML). The repo currently doesn't check in generated .html outputs under Examples/, so committing this will create large, noisy diffs over time and can go stale as the generator output changes. Consider removing this HTML file from the repo and leaving only the .ps1 example (or document how to generate the HTML locally).

Copilot uses AI. Check for mistakes.
@claude
Copy link

claude bot commented Mar 6, 2026

PR Review: Add locale decimal condition examples

Overall this is a welcome addition. The PowerShell 0,2 array-pitfall is a genuine user trap, and the documentation and example improvements are clear and accurate.


Public/New-TableCondition.ps1 - PASS

The .PARAMETER Value expansion and the two new .EXAMPLE blocks are a real improvement over the old placeholder text. The explanation of the dot-vs-comma issue is concise and correct.


Examples/Example-TableColumnHighlight/Example-LocaleDecimals.ps1 - PASS (minor notes)

The four-scenario matrix (HTML/JS store x dot/comma threshold) is a solid teaching tool.

Minor observations:

  • htmlDot uses string values like '0.17' rather than numeric literals. Fine for HTML-store tables, but a brief comment would help readers understand why strings are used here while jsDot uses numerics.
  • jsComma mixes string data with -DataStore JavaScript. Intentional, but a one-liner explaining the pairing would make the intent self-evident.

Examples/Example-TableColumnHighlight/Example-LocaleDecimals.html - WARNING

Generated HTML is checked in with a hardcoded meta-revised timestamp (2026-03-06 15:54:04). Two concerns:

  1. Stale immediately after merge - any future change to the .ps1 requires manually regenerating and recommitting the .html.
  2. Noise in diffs - future PRs regenerating examples will show large HTML diffs masking real changes.

If the repo convention is to commit generated HTML (other files in this directory appear to be committed), this is acceptable. If not, this file should be removed or added to .gitignore.


Tests/New-TableCondition.Tests.ps1 - Test 1 PASS, Test 2 WARNING

Test 1: Serializes dot and quoted comma thresholds

Solid. The three Should -Match assertions verify correct JSON serialization for both decimal formats. This directly tests PSWriteHTML behaviour and will catch regressions.

Test 2: Receives bare comma decimals as an array

Several concerns:

  1. Tests PowerShell parser, not PSWriteHTML. The assertions on bareCommaValue verify that 0,2 is an Object[] in PowerShell - this always passes regardless of any PSWriteHTML change.

  2. Fragile internal coupling. Accessing conditionWithBareComma.Output.Value couples the test to the internal object structure. If that shape changes, the test breaks without any real regression having occurred.

  3. TemporaryConditionArray.html is never validated. It is created and deleted but no assertion reads it.

  4. No assertion on library behaviour. The test does not verify whether PSWriteHTML warns, throws, or silently serialises the array input. There is no PSWriteHTML contract being guarded here.

  5. Cleanup runs inline rather than in AfterEach. If an assertion throws before the cleanup block, the temp file is left behind. AfterEach or try/finally ensures cleanup runs even on failure.

Suggestion: Either remove Test 2 (the docs already explain the pitfall clearly) or refactor to assert observable library behaviour - for example verifying the serialized HTML does not contain a valid numeric condition when an array is passed, rather than inspecting internal object properties.


Summary

Area Status
Documentation (New-TableCondition.ps1) PASS - clear improvement
Example script PASS - minor clarity opportunity
Generated HTML artifact WARNING - stale timestamp; confirm commit policy
Test 1 PASS - solid serialization guard
Test 2 WARNING - tests PS parser internals; fragile coupling; no library assertion

The core change is valuable and the documentation is ready to merge. Recommend either dropping Test 2 or replacing it with a library-behaviour assertion before merging.

@PrzemyslawKlys PrzemyslawKlys merged commit 3077dcc into master Mar 6, 2026
6 checks passed
@PrzemyslawKlys PrzemyslawKlys deleted the codex/locale-decimal-examples branch March 6, 2026 22:18
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.

2 participants