[CmdPal] Add number separators to calculator results - #49375
debaditya (DebadityaHait) wants to merge 2 commits into
Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
@microsoft-github-policy-service agree |
Michael Jolley (michaeljolley)
left a comment
There was a problem hiding this comment.
- Non-3-digit grouping cultures: (e.g. Indian hi-IN, group sep , == list sep , → strict token pattern \d{1,3}(?:[,]\d{3})+). N formatting yields 1,23,45,678, which that pattern won't match, so continuing a calculation from a replaced/= result can misparse. Edge case, but it's newly reachable because the app now emits these grouped strings.
- Test coverage gap: no test for the replace/= continuation path. A small regression test (type a >1000 value, trigger replace-to-search-box, append +1, assert the result) would lock in that the grouped Title round-trips.
60b0cdd to
a2bb745
Compare
|
Addressed the review feedback. Calculator continuation now uses the ungrouped operational result while retaining the culture-grouped display title. Added Validation: all 451 calculator unit tests pass. |
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
[edited/redacted by Jay]
Note that e.g. Chinese, Japanese, Korean, Vietnamese grouping is done in groups of four: 123,456 (Western) = 12,3456 (Chinese)
There are also several different characters being used globally. Make sure everything is correct.
|
Thanks for raising this. I double-checked the implementation and it does not hardcode Western three-digit grouping or separator characters. It uses .NET's Current .NET locale data uses three-digit separator grouping for I can make this clearer with focused tests covering a four-digit |
|
Michael Jolley (@michaeljolley), when you have a chance, could you confirm whether following .NET's |
|
I think this should be optional. Enabled by default - fine, but I'd like to turn it off, so I don't have to spend time deleting those separators. |
|
Woul
Would it be feasible to have this feature as "display-only", so copying the result only copies the numeric value and not the result string? I may be mistaken, but I think we may have discussed this on one of the earlier aborted attempts at doing this. |
|
Yes — that is already the intended behavior in this PR. Digit grouping is display-only; copying or pasting a result uses the ungrouped numeric value, and continued calculations also use that raw value. |
|
Closing this as stale. No activity in a month and it doesn't build. |
Summary of the Pull Request
Adds culture-aware digit grouping to visible Command Palette calculator result titles. Values used for copy, paste, suggestions, history, and reparsing remain raw.
PR Checklist
Detailed Description of the Pull Request / Additional comments
Calculator results previously used the same ungrouped string for both the visible title and operational values. This change derives a culture-aware, display-only string while preserving the existing decimal precision. Page and fallback result titles use the grouped string; command text and
TextToSuggestremain unformatted.Tests cover
en-USandde-DEgrouping and verify that copy and suggestion values remain raw.Validation Steps Performed
Microsoft.CmdPal.Ext.Calc.UnitTestsfor x64 Debug with the repository build script500000query displayed500,000while retaining raw500000operational textWindows UI evidence
The raw query
500000displays the grouped result title500,000while retaining raw operational text500000.