Backout 'smart' digit separation & Fix Mages functions for Calculator plugin #3971
Conversation
Mages did not like the previous change to smart thousands / decimal so backed that out. Workaround for FlorianRappl/Mages#132
|
🥷 Code experts: VictoriousRaptor, jjw24 VictoriousRaptor, jjw24 have most 👩💻 activity in the files. See details
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame: ✨ Comment |
|
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
📝 WalkthroughWalkthroughRefactors calculator plugin to be stateless and culture-aware: adds function detection and pow/log/ln rewrites, improves number normalization and formatting with configurable decimal/group separators, updates engine result mapping and copy-to-clipboard behaviour, introduces ShowErrorMessage setting/UI, regex updates, and adds unit tests. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant PL as Calculator Plugin
participant RH as Regex Helpers
participant EN as Expression Engine
U->>PL: Query(text)
PL->>PL: Trim + check empty/whitespace
alt empty/whitespace
PL-->>U: NotComplete / EmptyResults (depends on ShowErrorMessage)
else
PL->>RH: FunctionRegex.IsMatch(text)
RH-->>PL: isFunctionPresent
PL->>PL: GetDecimalSeparator/GetGroupSeparator
PL->>PL: NormalizeNumber(text, isFunctionPresent, decimalSep, groupSep)
loop rewrite nested pow (RightToLeft)
PL->>RH: PowRegex.Match
RH-->>PL: pow match
PL->>PL: PowMatchEvaluator -> replace pow(...) with (a^b)
end
loop rewrite log/ln
PL->>RH: LogRegex / LnRegex.Match
RH-->>PL: match
PL->>PL: Log/LnMatchEvaluator -> rewrite to log10()/log()
end
PL->>EN: Evaluate(rewrittenExpression)
EN-->>PL: result
alt null / Function / empty
PL-->>U: NotComplete-like result (with icon) or EmptyResults
else NaN
PL-->>U: Localized "Not a number"
else numeric/string
PL->>PL: Round to MaxDecimalPlaces & FormatResult
PL-->>U: Result (Title, SubTitle, Copy action, CopyText)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (10)
Plugins/Flow.Launcher.Plugin.Calculator/MainRegexHelper.cs (1)
11-12: Include locale space-separators (NBSP/narrow NBSP, apostrophe) in number regex.This helps parse inputs like “12 345”, “12 345”, or “12'345” that many locales produce, while still supporting negatives.
- [GeneratedRegex(@"-?[\d\.,]+", RegexOptions.Compiled)] + // Allow NBSP (\u00A0), narrow NBSP (\u202F), and apostrophe (') as group separators. + [GeneratedRegex(@"-?[\d\.,\u00A0\u202F']+", RegexOptions.Compiled)] public static partial Regex GetNumberRegex();Plugins/Flow.Launcher.Plugin.Calculator/plugin.json (1)
5-5: Fix description punctuation and wording.Double period and minor grammar; tighten phrasing.
- "Description": "Perform mathematical calculations, including hex values and advanced math functions, such as 'min(1,2,3)', 'sqrt(123)', 'cos(123)', etc.. User locale determines thousand separator and decimal place.", + "Description": "Perform mathematical calculations, including hex values and advanced math functions such as 'min(1,2,3)', 'sqrt(123)', 'cos(123)', etc. User locale determines the thousands and decimal separators.",Plugins/Flow.Launcher.Plugin.Calculator/Main.cs (8)
53-54: Consider NBSP/narrow NBSP grouping in NumberRegex matches.If users paste results containing “12 345” or “12 345”, current regex won’t match them as a single token. The proposed regex change in MainRegexHelper.cs addresses this. Ensure both files land together.
69-80: Avoid repeated ToString() and handle Infinity explicitly.Cache once and guard NaN/Infinity across numeric types. Minor perf/readability win.
- if (result == null || string.IsNullOrEmpty(result.ToString())) + var resultStr = result?.ToString(); + if (string.IsNullOrEmpty(resultStr)) { return new List<Result> { new Result { Title = Localize.flowlauncher_plugin_calculator_expression_not_complete(), IcoPath = "Images/calculator.png" } }; }And just below:
- if (result.ToString() == "NaN") + if ((result is double dd && (double.IsNaN(dd) || double.IsInfinity(dd))) || + (result is float ff && (float.IsNaN(ff) || float.IsInfinity(ff))) || + string.Equals(resultStr, "NaN", StringComparison.Ordinal)) result = Localize.flowlauncher_plugin_calculator_not_a_number();
87-91: Guard decimal conversion for large exponents.Convert.ToDecimal can overflow for big results (e.g., 2^1000). Consider falling back to invariant string formatting when conversion fails.
Example pattern:
decimal roundedResult; try { roundedResult = Math.Round(Convert.ToDecimal(result), _settings.MaxDecimalPlaces, MidpointRounding.AwayFromZero); } catch (OverflowException) { return ResultFromString(resultStr); } // formatting path without decimal
138-173: Minor fixes: use cached decimalSep; strip NBSP variants when normalizing.
- Use the local decimalSep variable consistently.
- Remove both NBSP and narrow NBSP groupers inside matched numbers to accommodate pasted locale output.
- if (i == parts.Length - 1 && part.Contains(culture.NumberFormat.NumberDecimalSeparator)) + if (i == parts.Length - 1 && part.Contains(decimalSep)) { - part = part.Split(culture.NumberFormat.NumberDecimalSeparator)[0]; + part = part.Split(decimalSep)[0]; }- string processedStr = numberStr.Replace(groupSep, ""); - processedStr = processedStr.Replace(decimalSep, "."); + string processedStr = numberStr.Replace(groupSep, ""); + // Also strip common space-based group separators if present. + processedStr = processedStr.Replace("\u00A0", "").Replace("\u202F", ""); + processedStr = processedStr.Replace(decimalSep, "."); return processedStr;
175-197: Locale-aware grouping sizes (nice-to-have).Indian locales (e.g., hi-IN) use 3,2 grouping. If UseSystemLocale is selected, consider NumberFormatInfo.NumberGroupSizes for insertion instead of regex.
199-209: Group separator selection: clarify behavior in UI.When not using system locale, you flip between “.” and “,”. Document in settings tooltip that grouping will inverse the chosen decimal separator.
213-224: Pre-validation is very permissive now; that’s fine, but add quick length guard.Optional: short-circuit extremely long inputs to avoid heavy regex work during Replace.
238-255: Bracket completeness doesn’t detect mis-nesting.“([)]” passes. Use a small stack to enforce proper pairing; still cheap.
Example:
private static bool IsBracketCompleteStrict(string s) { var st = new Stack<char>(); foreach (var c in s) { if (c is '(' or '[') st.Push(c); else if (c is ')' or ']') { if (st.Count == 0) return false; var open = st.Pop(); if ((c == ')' && open != '(') || (c == ']' && open != '[')) return false; } } return st.Count == 0; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Plugins/Flow.Launcher.Plugin.Calculator/Main.cs(4 hunks)Plugins/Flow.Launcher.Plugin.Calculator/MainRegexHelper.cs(1 hunks)Plugins/Flow.Launcher.Plugin.Calculator/plugin.json(1 hunks)
🧰 Additional context used
🪛 GitHub Check: Check Spelling
Plugins/Flow.Launcher.Plugin.Calculator/Main.cs
[warning] 63-63:
bpow is not a recognized word. (unrecognized-spelling)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: gitStream.cm
- GitHub Check: build
Previous solution missed e.g. `pow(min(2,3), 4)`
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
Plugins/Flow.Launcher.Plugin.Calculator/Main.cs (4)
19-20: Harden pow-regex: add timeout and CultureInvariant; silence linter noise.Current PowRegex has no match timeout and uses culture-sensitive boundaries, which risks pathological inputs hanging the UI and triggers the spelling checker's “bpow” warning. Safer to use a culture-invariant boundary and a small timeout.
- private static readonly Regex PowRegex = new(@"\bpow(\((?:[^()\[\]]|\((?<Depth>)|\)(?<-Depth>)|\[(?<Depth>)|\](?<-Depth>))*(?(Depth)(?!))\))", RegexOptions.Compiled | RegexOptions.RightToLeft); + private static readonly Regex PowRegex = new( + @"(?<!\w)pow(\((?:[^()\[\]]|\((?<Depth>)|\)(?<-Depth>)|\[(?<Depth>)|\](?<-Depth>))*(?(Depth)(?!))\))", + RegexOptions.Compiled | RegexOptions.CultureInvariant | RegexOptions.RightToLeft, + TimeSpan.FromMilliseconds(100));
83-94: Minor: avoid repeated ToString() calls.Cache result.ToString() once to reduce allocations and double evaluation.
- if (result == null || string.IsNullOrEmpty(result.ToString())) + var resultStr = result?.ToString(); + if (string.IsNullOrEmpty(resultStr)) { ... }
95-100: Use type checks for NaN/Infinity instead of string comparison.Comparing ToString() to "NaN" is brittle. Prefer numeric checks.
- if (result.ToString() == "NaN") - result = Localize.flowlauncher_plugin_calculator_not_a_number(); + if (result is double d && (double.IsNaN(d) || double.IsInfinity(d))) + result = Localize.flowlauncher_plugin_calculator_not_a_number();
240-274: Consider using NumberFormatInfo for locale-correct grouping (e.g., Indian 2-2-3).Regex-based 3-digit grouping won’t match all locales. You can render via a tailored NumberFormatInfo to respect both separators and group sizes.
Example approach (outside this range):
- Build a NumberFormatInfo based on system when UseSystemLocale, else clone Invariant and set NumberDecimalSeparator/NumberGroupSeparator.
- Format with roundedResult.ToString("N" + _settings.MaxDecimalPlaces, nfi).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Plugins/Flow.Launcher.Plugin.Calculator/Main.cs(6 hunks)
🧰 Additional context used
🪛 GitHub Check: Check Spelling
Plugins/Flow.Launcher.Plugin.Calculator/Main.cs
[warning] 19-19:
bpow is not a recognized word. (unrecognized-spelling)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: gitStream.cm
- GitHub Check: build
🔇 Additional comments (5)
Plugins/Flow.Launcher.Plugin.Calculator/Main.cs (5)
48-63: Nice UX: early-return with “expression not complete”.Bracket-completeness guard reduces noisy errors and guides the user well.
69-79: Bracket-aware pow(x,y) rewrite: good fix for Mages bug.Handles nesting and commas correctly; loop converges. Once Mages fixes pow, this remains harmless.
117-127: Clipboard action with exception handling looks good.Robust copy flow and user feedback are well handled.
148-197: PowMatchEvaluator logic LGTM.Top-level comma detection with bracket depth across () and [] is correct; malformed cases safely fall back.
288-310: Balanced-bracket scan is straightforward and safe.Immediate negative check prevents false completeness; good.
Jack251970
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
Plugins/Flow.Launcher.Plugin.Calculator/MainRegexHelper.cs (1)
16-17: Pow(...) detector is solid; add CultureInvariant for stability.Add RegexOptions.CultureInvariant to avoid any culture-specific word-boundary quirks.
- [GeneratedRegex(@"\bpow(\((?:[^()\[\]]|\((?<Depth>)|\)(?<-Depth>)|\[(?<Depth>)|\](?<-Depth>))*(?(Depth)(?!))\))", RegexOptions.Compiled | RegexOptions.RightToLeft)] + [GeneratedRegex(@"\bpow(\((?:[^()\[\]]|\((?<Depth>)|\)(?<-Depth>)|\[(?<Depth>)|\](?<-Depth>))*(?(Depth)(?!))\))", RegexOptions.Compiled | RegexOptions.RightToLeft | RegexOptions.CultureInvariant)]Plugins/Flow.Launcher.Plugin.Calculator/Main.cs (6)
65-65: Avoid per-match culture lookups in NormalizeNumber.Cache separators once and pass into the evaluator to reduce allocations/GC in long inputs.
- var expression = NumberRegex.Replace(query.Search, m => NormalizeNumber(m.Value)); + var culture = CultureInfo.CurrentCulture; + var groupSep = culture.NumberFormat.NumberGroupSeparator; + var decimalSep = culture.NumberFormat.NumberDecimalSeparator; + var expression = NumberRegex.Replace(query.Search, m => NormalizeNumber(m.Value, groupSep, decimalSep));And change signature accordingly (see lines 197-206).
93-95: Use type checks for NaN/Infinity instead of string comparison.To avoid culture/format surprises, check numeric types directly; also handle ±Infinity to prevent Convert.ToDecimal exceptions.
- if (result.ToString() == "NaN") - result = Localize.flowlauncher_plugin_calculator_not_a_number(); + if (result is double d && double.IsNaN(d)) + result = Localize.flowlauncher_plugin_calculator_not_a_number(); + else if (result is double d2 && double.IsInfinity(d2)) + result = Localize.flowlauncher_plugin_calculator_expression_not_complete();
132-140: Swallowing all exceptions hides diagnostics; add a debug trace.Log the exception (at least in debug) to aid field investigations; keep user-facing message unchanged.
- catch (Exception) + catch (Exception ex) { + System.Diagnostics.Debug.WriteLine($"Calculator evaluation error: {ex}"); // Mages engine can throw various exceptions, for simplicity we catch them all and show a generic message.
146-196: PowMatchEvaluator: handles nesting and commas correctly.Good bracket-depth split. One edge: if someone passes 3+ args (unexpected), we currently fold arg2..n into the right side. Consider bailing out when additional top‑level commas exist in arg2 to defer to Mages’ error.
- var arg2 = argsContent.Substring(splitIndex + 1).Trim(); + var arg2 = argsContent.Substring(splitIndex + 1).Trim(); + // If there is another top-level comma in arg2, treat as non-2-arg pow and bail. + var depth = 0; + for (int i = 0; i < arg2.Length; i++) + { + var c = arg2[i]; + if (c == '(' || c == '[') depth++; + else if (c == ')' || c == ']') depth--; + else if (c == ',' && depth == 0) return m.Value; + }
197-236: NormalizeNumber: keep validation; pass separators as params for clarity/perf.Intent (use system locale for input; plugin setting for output) is sound and preserves min(2,3). Make separators explicit to document the contract and avoid repeated CultureInfo lookups.
- private static string NormalizeNumber(string numberStr) + private static string NormalizeNumber(string numberStr, string groupSep, string decimalSep) { - var culture = CultureInfo.CurrentCulture; - var groupSep = culture.NumberFormat.NumberGroupSeparator; - var decimalSep = culture.NumberFormat.NumberDecimalSeparator; + // Input normalization intentionally uses system locale separators.Also update the decimal check inside the loop to use the passed-in decimalSep.
- if (i == parts.Length - 1 && part.Contains(culture.NumberFormat.NumberDecimalSeparator)) + if (i == parts.Length - 1 && part.Contains(decimalSep))
286-308: Minor: variable name nit.Rename “matchs” → “matches”.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Plugins/Flow.Launcher.Plugin.Calculator/Languages/en.xaml(1 hunks)Plugins/Flow.Launcher.Plugin.Calculator/Main.cs(6 hunks)Plugins/Flow.Launcher.Plugin.Calculator/MainRegexHelper.cs(1 hunks)Plugins/Flow.Launcher.Plugin.Calculator/plugin.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Plugins/Flow.Launcher.Plugin.Calculator/plugin.json
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: dcog989
PR: Flow-Launcher/Flow.Launcher#3971
File: Plugins/Flow.Launcher.Plugin.Calculator/Main.cs:67-67
Timestamp: 2025-09-13T12:50:05.270Z
Learning: In Flow.Launcher Calculator plugin, NormalizeNumber must validate thousand separator usage before stripping them. If a comma-separated token doesn't follow proper thousand separator rules (3 digits between separators), it should be left unmodified to preserve function arguments like in "min(2,3)".
Learnt from: dcog989
PR: Flow-Launcher/Flow.Launcher#3971
File: Plugins/Flow.Launcher.Plugin.Calculator/Main.cs:67-67
Timestamp: 2025-09-13T12:50:05.271Z
Learning: NumberRegex pattern `-?[\d\.,]+` in Flow.Launcher Calculator will match function arguments like `2,3` in `min(2,3)`. The NormalizeNumber validation that checks for proper 3-digit thousand separator grouping is essential to avoid breaking function calls by incorrectly stripping commas from function arguments.
📚 Learning: 2025-09-13T12:50:05.271Z
Learnt from: dcog989
PR: Flow-Launcher/Flow.Launcher#3971
File: Plugins/Flow.Launcher.Plugin.Calculator/Main.cs:67-67
Timestamp: 2025-09-13T12:50:05.271Z
Learning: NumberRegex pattern `-?[\d\.,]+` in Flow.Launcher Calculator will match function arguments like `2,3` in `min(2,3)`. The NormalizeNumber validation that checks for proper 3-digit thousand separator grouping is essential to avoid breaking function calls by incorrectly stripping commas from function arguments.
Applied to files:
Plugins/Flow.Launcher.Plugin.Calculator/MainRegexHelper.csPlugins/Flow.Launcher.Plugin.Calculator/Main.cs
📚 Learning: 2025-09-13T12:50:05.270Z
Learnt from: dcog989
PR: Flow-Launcher/Flow.Launcher#3971
File: Plugins/Flow.Launcher.Plugin.Calculator/Main.cs:67-67
Timestamp: 2025-09-13T12:50:05.270Z
Learning: In Flow.Launcher Calculator plugin, NormalizeNumber must validate thousand separator usage before stripping them. If a comma-separated token doesn't follow proper thousand separator rules (3 digits between separators), it should be left unmodified to preserve function arguments like in "min(2,3)".
Applied to files:
Plugins/Flow.Launcher.Plugin.Calculator/Main.cs
🧬 Code graph analysis (1)
Plugins/Flow.Launcher.Plugin.Calculator/Main.cs (1)
Plugins/Flow.Launcher.Plugin.Calculator/MainRegexHelper.cs (1)
MainRegexHelper(5-18)
🪛 GitHub Check: Check Spelling
Plugins/Flow.Launcher.Plugin.Calculator/MainRegexHelper.cs
[warning] 16-16:
bpow is not a recognized word. (unrecognized-spelling)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (8)
Plugins/Flow.Launcher.Plugin.Calculator/Languages/en.xaml (1)
7-7: Description update looks good; ensure other locales are synced.Please propagate this wording to all localized xaml/json resources to avoid inconsistent plugin descriptions across languages.
Plugins/Flow.Launcher.Plugin.Calculator/MainRegexHelper.cs (1)
10-11: Number regex change to allow leading minus: LGTM.Covers negatives without altering existing grouping/decimal handling.
Plugins/Flow.Launcher.Plugin.Calculator/Main.cs (6)
46-50: Early empty-query return: LGTM.
51-61: Bracket completeness UX: LGTM.Nice guard; prevents noisy exceptions from the engine.
67-77: Pow rewrite: correct and bracket-aware.Loop with RightToLeft + evaluator handles nesting; termination is guaranteed. Nice.
81-92: Null/empty result handling: LGTM.
99-103: Result formatting path: LGTM.Rounding then culture-aware formatting is consistent.
238-272: Formatting: negative numbers and grouping — regex is fine.Regex-based grouping works with negatives via \B; fallback mapping of group sep vs decimal sep is reasonable.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Plugins/Flow.Launcher.Plugin.Calculator/Main.cs (1)
81-116: Bug: converting string results to decimal throws and masks intended messages.After mapping NaN/function results to localized strings, the code still runs Convert.ToDecimal(result) which will throw, hit the catch, and show the generic “expression not complete” instead of the intended message. Handle these cases with early returns and only format numeric results.
- if (result.ToString() == "NaN") - result = Localize.flowlauncher_plugin_calculator_not_a_number(); - - if (result is Function) - result = Localize.flowlauncher_plugin_calculator_expression_not_complete(); - - if (!string.IsNullOrEmpty(result.ToString())) - { - decimal roundedResult = Math.Round(Convert.ToDecimal(result), _settings.MaxDecimalPlaces, MidpointRounding.AwayFromZero); - string newResult = FormatResult(roundedResult); + // Handle non-numeric outcomes first + if (result is double d && (double.IsNaN(d) || double.IsInfinity(d))) + { + return new List<Result> + { + new Result { Title = Localize.flowlauncher_plugin_calculator_not_a_number(), IcoPath = "Images/calculator.png" } + }; + } + if (result is Function) + { + return new List<Result> + { + new Result { Title = Localize.flowlauncher_plugin_calculator_expression_not_complete(), IcoPath = "Images/calculator.png" } + }; + } + + // Numeric formatting path + if (!string.IsNullOrEmpty(result.ToString())) + { + decimal roundedResult = Math.Round(Convert.ToDecimal(result, CultureInfo.InvariantCulture), _settings.MaxDecimalPlaces, MidpointRounding.AwayFromZero); + string newResult = FormatResult(roundedResult);
🧹 Nitpick comments (4)
Plugins/Flow.Launcher.Plugin.Calculator/Main.cs (4)
53-66: Bracket-aware pow() rewrite: solid approach; loop likely unnecessary.Using a balancing‑group regex plus a top‑level comma splitter fixes nested cases (e.g., pow(min(2,3),4)) reliably. Since Regex.Replace processes all matches, the do/while is probably redundant and can be removed for a tiny perf/readability win.
- string previous; - do - { - previous = expression; - expression = PowRegex.Replace(previous, PowMatchEvaluator); - } while (previous != expression); + expression = PowRegex.Replace(expression, PowMatchEvaluator);
134-183: Guard against extra top-level commas in pow(...) to avoid malformed rewrites.pow(2,3,4) should not rewrite to “(2^3,4)”. If more than one top‑level comma exists, leave the match unchanged and let Mages error out.
- // Find the top-level comma that separates the two arguments of pow. + // Find the top-level comma that separates the two arguments of pow. for (var i = 0; i < argsContent.Length; i++) { switch (argsContent[i]) { case '(': case '[': bracketCount++; break; case ')': case ']': bracketCount--; break; case ',' when bracketCount == 0: splitIndex = i; break; } if (splitIndex != -1) break; } if (splitIndex == -1) { // This indicates malformed arguments for pow, e.g., pow(5) or pow(). // Return original string to let Mages handle the error. return m.Value; } var arg1 = argsContent.Substring(0, splitIndex).Trim(); - var arg2 = argsContent.Substring(splitIndex + 1).Trim(); + var arg2 = argsContent.Substring(splitIndex + 1).Trim(); + + // Ensure there is no second top-level comma (i.e., exactly two args) + bracketCount = 0; + for (var i = splitIndex + 1; i < argsContent.Length; i++) + { + var c = argsContent[i]; + if (c == '(' || c == '[') bracketCount++; + else if (c == ')' || c == ']') bracketCount--; + else if (c == ',' && bracketCount == 0) return m.Value; // >2 args: don't rewrite + }
189-224: Input normalization uses OS locale only; confirm intent vs plugin DecimalSeparator setting.NormalizeNumber relies on CultureInfo.CurrentCulture. If a user selects DecimalSeparator=Comma on an en‑US system, inputs like “1,23+4” will likely not evaluate as intended because commas are preserved to avoid breaking function args. If the feature goal is “output honors setting, input honors OS locale,” please document this; otherwise, consider aligning input normalization with the plugin setting while preserving the “3‑digit thousand grouping” safeguard.
Would you like a patch that respects the plugin DecimalSeparator for input only when the token is an unambiguous decimal (e.g., contains a single decimal sep and no group seps), falling back to “leave as-is” when ambiguous (like “2,3”)?
226-248: Large number/precision path: avoid decimal overflow and preserve precision.Convert.ToDecimal(double) can overflow for very large magnitudes, falling into the catch and showing a generic message. Consider:
- If result is decimal -> round directly.
- If result is double -> handle NaN/Infinity earlier (already suggested) and use decimal.TryParse from invariant string as fallback; if that fails, format as double with invariant and skip grouping/rounding.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Plugins/Flow.Launcher.Plugin.Calculator/Main.cs(5 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: dcog989
PR: Flow-Launcher/Flow.Launcher#3971
File: Plugins/Flow.Launcher.Plugin.Calculator/Main.cs:67-67
Timestamp: 2025-09-13T12:50:05.270Z
Learning: In Flow.Launcher Calculator plugin, NormalizeNumber must validate thousand separator usage before stripping them. If a comma-separated token doesn't follow proper thousand separator rules (3 digits between separators), it should be left unmodified to preserve function arguments like in "min(2,3)".
Learnt from: dcog989
PR: Flow-Launcher/Flow.Launcher#3971
File: Plugins/Flow.Launcher.Plugin.Calculator/Main.cs:67-67
Timestamp: 2025-09-13T12:50:05.271Z
Learning: NumberRegex pattern `-?[\d\.,]+` in Flow.Launcher Calculator will match function arguments like `2,3` in `min(2,3)`. The NormalizeNumber validation that checks for proper 3-digit thousand separator grouping is essential to avoid breaking function calls by incorrectly stripping commas from function arguments.
📚 Learning: 2025-09-13T12:50:05.270Z
Learnt from: dcog989
PR: Flow-Launcher/Flow.Launcher#3971
File: Plugins/Flow.Launcher.Plugin.Calculator/Main.cs:67-67
Timestamp: 2025-09-13T12:50:05.270Z
Learning: In Flow.Launcher Calculator plugin, NormalizeNumber must validate thousand separator usage before stripping them. If a comma-separated token doesn't follow proper thousand separator rules (3 digits between separators), it should be left unmodified to preserve function arguments like in "min(2,3)".
Applied to files:
Plugins/Flow.Launcher.Plugin.Calculator/Main.cs
📚 Learning: 2025-09-13T12:50:05.271Z
Learnt from: dcog989
PR: Flow-Launcher/Flow.Launcher#3971
File: Plugins/Flow.Launcher.Plugin.Calculator/Main.cs:67-67
Timestamp: 2025-09-13T12:50:05.271Z
Learning: NumberRegex pattern `-?[\d\.,]+` in Flow.Launcher Calculator will match function arguments like `2,3` in `min(2,3)`. The NormalizeNumber validation that checks for proper 3-digit thousand separator grouping is essential to avoid breaking function calls by incorrectly stripping commas from function arguments.
Applied to files:
Plugins/Flow.Launcher.Plugin.Calculator/Main.cs
🧬 Code graph analysis (1)
Plugins/Flow.Launcher.Plugin.Calculator/Main.cs (1)
Plugins/Flow.Launcher.Plugin.Calculator/MainRegexHelper.cs (1)
MainRegexHelper(5-18)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: gitStream.cm
- GitHub Check: build
🔇 Additional comments (2)
Plugins/Flow.Launcher.Plugin.Calculator/Main.cs (2)
18-18: PowRegex field addition looks good.Localizing the pattern into MainRegexHelper and caching it here is a good move for perf and cohesion.
46-46: Early return on empty/whitespace query is fine.Prevents unnecessary engine calls.
|
@dcog989 I removed I have observed one issue here - some expressions cannot work in the latest commit of this PR. 
Original behaviour: 
|
@Jack251970 - what region / language / number format are you using? |
This is my language & region setting. 
|
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Plugins/Flow.Launcher.Plugin.Calculator/Main.cs (1)
301-323: Use NumberFormatInfo for culture-correct grouping (e.g., hi-IN) and avoid padding zeros.Regex-based 3-digit grouping won’t honor locales with non-3 group sizes. Format with NFI, then trim trailing fractional zeros.
Apply this diff:
- private string FormatResult(decimal roundedResult) - { - string decimalSeparator = GetDecimalSeparator(); - string groupSeparator = GetGroupSeparator(decimalSeparator); - - string resultStr = roundedResult.ToString(CultureInfo.InvariantCulture); - - string[] parts = resultStr.Split('.'); - string integerPart = parts[0]; - string fractionalPart = parts.Length > 1 ? parts[1] : string.Empty; - - if (integerPart.Length > 3) - { - integerPart = ThousandGroupRegex.Replace(integerPart, groupSeparator); - } - - if (!string.IsNullOrEmpty(fractionalPart)) - { - return integerPart + decimalSeparator + fractionalPart; - } - - return integerPart; - } + private string FormatResult(decimal roundedResult) + { + var nfi = (NumberFormatInfo)CultureInfo.CurrentCulture.NumberFormat.Clone(); + nfi.NumberDecimalSeparator = GetDecimalSeparator(); + nfi.NumberGroupSeparator = GetGroupSeparator(nfi.NumberDecimalSeparator); + + var s = roundedResult.ToString("N" + _settings.MaxDecimalPlaces, nfi); + // Trim trailing fractional zeros to match previous behavior + if (_settings.MaxDecimalPlaces > 0 && s.Contains(nfi.NumberDecimalSeparator)) + { + // Remove trailing zeros, then remove decimal separator if now trailing + int i = s.Length - 1; + while (i >= 0 && s[i] == '0') i--; + if (i >= 0 && s[i].ToString() == nfi.NumberDecimalSeparator) i--; + s = s.Substring(0, i + 1); + } + return s; + }
♻️ Duplicate comments (1)
Plugins/Flow.Launcher.Plugin.Calculator/Main.cs (1)
56-65: Global isFunctionPresent gate may leave valid grouped numbers un-normalized.Example to double‑check: min(2,3)+1,234 should still parse 1,234 → 1234 even when a function exists elsewhere. If this already passes with Mages in your tests, ignore; otherwise consider per‑token validation (strip only when grouping is valid).
You can quickly sanity‑check behavior with these expressions:
- min(2,3)+1,234
- max(1, 2,3)
- pow(min(2,3), 4)
🧹 Nitpick comments (1)
Plugins/Flow.Launcher.Plugin.Calculator/Main.cs (1)
202-243: Fix: dot-grouping validation should ignore the decimal fraction before IsValidGrouping.With cultures like 1.234.567,89 (group '.'; decimal ','), parts[^1] contains the decimal part, causing IsValidGrouping to fail and dots not to be stripped, then replacing ','→'.' yields an invalid 1.234.567.89. Strip the decimal portion from the last group before validation.
Apply this diff inside the '.' branch:
- var parts = processedStr.Split('.'); - if (parts.Length > 1) - { - var culture = CultureInfo.CurrentCulture; - if (IsValidGrouping(parts, culture.NumberFormat.NumberGroupSizes)) - { - processedStr = processedStr.Replace(groupSep, ""); - } - // If not grouped, it's likely a decimal number, so we don't strip dots. - } + var parts = processedStr.Split('.'); + if (parts.Length > 1) + { + // Exclude decimal fraction from the last group before validating grouping. + var last = parts[^1]; + var decIdx = !string.IsNullOrEmpty(decimalSep) ? last.IndexOf(decimalSep, StringComparison.Ordinal) : -1; + if (decIdx >= 0) + parts[^1] = last[..decIdx]; + + var culture = CultureInfo.CurrentCulture; + if (IsValidGrouping(parts, culture.NumberFormat.NumberGroupSizes)) + { + processedStr = processedStr.Replace(groupSep, ""); + } + // If not grouped, it's likely a decimal number; don't strip dots. + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Plugins/Flow.Launcher.Plugin.Calculator/Main.cs(5 hunks)Plugins/Flow.Launcher.Plugin.Calculator/Settings.cs(1 hunks)Plugins/Flow.Launcher.Plugin.Calculator/ViewModels/SettingsViewModel.cs(1 hunks)Plugins/Flow.Launcher.Plugin.Calculator/Views/CalculatorSettings.xaml.cs(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: dcog989
PR: Flow-Launcher/Flow.Launcher#3971
File: Plugins/Flow.Launcher.Plugin.Calculator/Main.cs:67-67
Timestamp: 2025-09-13T12:50:05.270Z
Learning: In Flow.Launcher Calculator plugin, NormalizeNumber must validate thousand separator usage before stripping them. If a comma-separated token doesn't follow proper thousand separator rules (3 digits between separators), it should be left unmodified to preserve function arguments like in "min(2,3)".
Learnt from: dcog989
PR: Flow-Launcher/Flow.Launcher#3971
File: Plugins/Flow.Launcher.Plugin.Calculator/Main.cs:67-67
Timestamp: 2025-09-13T12:50:05.271Z
Learning: NumberRegex pattern `-?[\d\.,]+` in Flow.Launcher Calculator will match function arguments like `2,3` in `min(2,3)`. The NormalizeNumber validation that checks for proper 3-digit thousand separator grouping is essential to avoid breaking function calls by incorrectly stripping commas from function arguments.
📚 Learning: 2025-09-13T12:50:05.270Z
Learnt from: dcog989
PR: Flow-Launcher/Flow.Launcher#3971
File: Plugins/Flow.Launcher.Plugin.Calculator/Main.cs:67-67
Timestamp: 2025-09-13T12:50:05.270Z
Learning: In Flow.Launcher Calculator plugin, NormalizeNumber must validate thousand separator usage before stripping them. If a comma-separated token doesn't follow proper thousand separator rules (3 digits between separators), it should be left unmodified to preserve function arguments like in "min(2,3)".
Applied to files:
Plugins/Flow.Launcher.Plugin.Calculator/Settings.csPlugins/Flow.Launcher.Plugin.Calculator/Main.cs
📚 Learning: 2025-09-13T12:50:05.271Z
Learnt from: dcog989
PR: Flow-Launcher/Flow.Launcher#3971
File: Plugins/Flow.Launcher.Plugin.Calculator/Main.cs:67-67
Timestamp: 2025-09-13T12:50:05.271Z
Learning: NumberRegex pattern `-?[\d\.,]+` in Flow.Launcher Calculator will match function arguments like `2,3` in `min(2,3)`. The NormalizeNumber validation that checks for proper 3-digit thousand separator grouping is essential to avoid breaking function calls by incorrectly stripping commas from function arguments.
Applied to files:
Plugins/Flow.Launcher.Plugin.Calculator/Main.cs
🧬 Code graph analysis (3)
Plugins/Flow.Launcher.Plugin.Calculator/Views/CalculatorSettings.xaml.cs (2)
Plugins/Flow.Launcher.Plugin.Calculator/ViewModels/SettingsViewModel.cs (1)
SettingsViewModel(6-26)Plugins/Flow.Launcher.Plugin.Calculator/Settings.cs (1)
Settings(4-11)
Plugins/Flow.Launcher.Plugin.Calculator/ViewModels/SettingsViewModel.cs (2)
Plugins/Flow.Launcher.Plugin.Calculator/Settings.cs (1)
Settings(4-11)Plugins/Flow.Launcher.Plugin.Calculator/Main.cs (1)
List(47-149)
Plugins/Flow.Launcher.Plugin.Calculator/Main.cs (1)
Plugins/Flow.Launcher.Plugin.Calculator/MainRegexHelper.cs (1)
MainRegexHelper(5-18)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: gitStream.cm
- GitHub Check: build
🔇 Additional comments (8)
Plugins/Flow.Launcher.Plugin.Calculator/Settings.cs (1)
6-10: LGTM: settings surface looks clean and consistent.Defaults make sense; new ShowErrorMessage flag is wired across the UI per PR.
Plugins/Flow.Launcher.Plugin.Calculator/ViewModels/SettingsViewModel.cs (2)
6-8: Nice refactor to primary ctor + init-only property.Simple and idiomatic; no behavioral changes.
14-23: Setter change is correct (fires only on change).This avoids redundant notifications.
Plugins/Flow.Launcher.Plugin.Calculator/Views/CalculatorSettings.xaml.cs (1)
10-14: ViewModel initialization order is fine.Setting DataContext before InitializeComponent is OK; if any XAML uses x:Bind-like patterns (not typical in WPF), consider moving after. No action required.
Plugins/Flow.Launcher.Plugin.Calculator/Main.cs (4)
66-76: Pow rewrite: bracket-aware approach looks solid.Iterative Replace with the balanced‑parens regex plus evaluator resolves nested cases.
80-91: Graceful “incomplete” handling gated by ShowErrorMessage.Matches the new setting; good UX.
151-200: PowMatchEvaluator handles nesting and empty args.Good defensive checks; trims args; returns original on malformed input.
24-26: Icon path verified — no action required.
Images/calculator.png exists at Plugins/Flow.Launcher.Plugin.Calculator/Images/calculator.png (also at Flow.Launcher/Images/calculator.png); plugin.json and Main.cs reference the same Images/calculator.png path — icon will be loaded at runtime.
|
@Jack251970 - thanks for your help / additions to this. Looks good but lots of 'moving parts' with this one so real world testing needed! |
Thanks for your suggestion! I resolved one issue with |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
Plugins/Flow.Launcher.Plugin.Calculator/Main.cs (1)
256-312: Strict comma rule blocks grouped numbers when any function exists.Tokens like “min(2,3)+1,234” won’t normalize the “1,234” part. Consider validating per‑token grouping (only preserve commas when grouping is invalid), not a global gate.
Would you like a focused diff that uses IsValidGrouping to decide per token?
🧹 Nitpick comments (6)
Plugins/Flow.Launcher.Plugin.Calculator/MainRegexHelper.cs (1)
7-8: Include curly apostrophe U+2019 in number-token regex.Many locales use U+2019 (’). Without it, tokens like 1’234 won’t normalize.
Apply:
- [GeneratedRegex(@"-?[\d\.,'\u00A0\u202F]+", RegexOptions.Compiled | RegexOptions.CultureInvariant)] + [GeneratedRegex(@"-?[\d\.,'\u2019\u00A0\u202F]+", RegexOptions.Compiled | RegexOptions.CultureInvariant)]Flow.Launcher.Test/Plugins/CalculatorTest.cs (2)
23-27: Make tests locale‑deterministic (set DecimalSeparator and precision).CI culture may not be dot-decimal; explicit settings avoid flaky expectations like “0.0000000000”.
Apply:
- settingField.SetValue(_plugin, new Settings - { - ShowErrorMessage = false // Make sure we return the empty results when error occurs - }); + settingField.SetValue(_plugin, new Settings + { + ShowErrorMessage = false, // return empty results on error + DecimalSeparator = DecimalSeparator.Dot, // deterministic output (matches expected test strings) + MaxDecimalPlaces = 10 + });
31-38: Avoid static engine leakage across fixtures.Since MagesEngine is static, consider resetting it after the fixture to prevent cross‑test interference.
Example:
@@ public CalculatorPluginTest() { @@ } + + [OneTimeTearDown] + public void TearDown() + { + var engineField = typeof(Main).GetField("MagesEngine", BindingFlags.NonPublic | BindingFlags.Static); + engineField?.SetValue(null, null); + }Plugins/Flow.Launcher.Plugin.Calculator/Main.cs (3)
119-127: Handle Infinity explicitly (parity with NaN).Division by zero may yield ±Infinity; treat like NaN for clearer UX instead of relying on Convert.ToDecimal throwing.
Apply:
- if (result.ToString() == "NaN") - { - result = Localize.flowlauncher_plugin_calculator_not_a_number(); - } + if ((result is double d && (double.IsNaN(d) || double.IsInfinity(d))) || + (result is float f && (float.IsNaN(f) || float.IsInfinity(f))) || + string.Equals(result.ToString(), "NaN", StringComparison.Ordinal)) + { + result = Localize.flowlauncher_plugin_calculator_not_a_number(); + }
355-377: Format with real culture group sizes (hi‑IN, etc.) while preserving fractional output.Current regex assumes 3‑digit groups. Use NumberFormatInfo to format the integer part, keep fractional part from the rounded decimal so tests like “sin(pi)” still show 10 zeros.
Apply:
- private string FormatResult(decimal roundedResult) - { - string decimalSeparator = GetDecimalSeparator(); - string groupSeparator = GetGroupSeparator(decimalSeparator); - - string resultStr = roundedResult.ToString(CultureInfo.InvariantCulture); - - string[] parts = resultStr.Split('.'); - string integerPart = parts[0]; - string fractionalPart = parts.Length > 1 ? parts[1] : string.Empty; - - if (integerPart.Length > 3) - { - integerPart = ThousandGroupRegex.Replace(integerPart, groupSeparator); - } - - if (!string.IsNullOrEmpty(fractionalPart)) - { - return integerPart + decimalSeparator + fractionalPart; - } - - return integerPart; - } + private string FormatResult(decimal roundedResult) + { + var decSep = GetDecimalSeparator(); + var grpSep = GetGroupSeparator(decSep); + + // Preserve fractional digits exactly as produced by the rounded decimal + // (e.g., "0.0000000000" for sin(pi)). + var invariant = roundedResult.ToString(CultureInfo.InvariantCulture); + var parts = invariant.Split('.'); + var fractional = parts.Length > 1 ? parts[1] : string.Empty; + + // Format the integer part with the culture’s real group sizes + var nfi = (NumberFormatInfo)CultureInfo.CurrentCulture.NumberFormat.Clone(); + nfi.NumberDecimalSeparator = decSep; + nfi.NumberGroupSeparator = grpSep; + var intGrouped = decimal.Truncate(roundedResult).ToString("N0", nfi); + + return string.IsNullOrEmpty(fractional) + ? intGrouped + : intGrouped + decSep + fractional; + }
239-241: Comment nit: “log(x) -> log10(x)” is common log, not natural.Adjust comment to avoid confusion.
Apply:
- // log(x) -> log10(x) (natural log) + // log(x) -> log10(x) (common/base‑10 log)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Flow.Launcher.Test/Flow.Launcher.Test.csproj(1 hunks)Flow.Launcher.Test/Plugins/CalculatorTest.cs(1 hunks)Plugins/Flow.Launcher.Plugin.Calculator/Main.cs(5 hunks)Plugins/Flow.Launcher.Plugin.Calculator/MainRegexHelper.cs(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: dcog989
PR: Flow-Launcher/Flow.Launcher#3971
File: Plugins/Flow.Launcher.Plugin.Calculator/Main.cs:67-67
Timestamp: 2025-09-13T12:50:05.270Z
Learning: In Flow.Launcher Calculator plugin, NormalizeNumber must validate thousand separator usage before stripping them. If a comma-separated token doesn't follow proper thousand separator rules (3 digits between separators), it should be left unmodified to preserve function arguments like in "min(2,3)".
Learnt from: dcog989
PR: Flow-Launcher/Flow.Launcher#3971
File: Plugins/Flow.Launcher.Plugin.Calculator/Main.cs:67-67
Timestamp: 2025-09-13T12:50:05.271Z
Learning: NumberRegex pattern `-?[\d\.,]+` in Flow.Launcher Calculator will match function arguments like `2,3` in `min(2,3)`. The NormalizeNumber validation that checks for proper 3-digit thousand separator grouping is essential to avoid breaking function calls by incorrectly stripping commas from function arguments.
📚 Learning: 2024-10-08T15:52:58.573Z
Learnt from: taooceros
PR: Flow-Launcher/Flow.Launcher#2616
File: Flow.Launcher/Flow.Launcher.csproj:7-7
Timestamp: 2024-10-08T15:52:58.573Z
Learning: In the Flow Launcher project, the version number in the `Flow.Launcher.csproj` file is dynamically updated during the CI/CD process.
Applied to files:
Flow.Launcher.Test/Flow.Launcher.Test.csproj
📚 Learning: 2025-09-13T12:50:05.271Z
Learnt from: dcog989
PR: Flow-Launcher/Flow.Launcher#3971
File: Plugins/Flow.Launcher.Plugin.Calculator/Main.cs:67-67
Timestamp: 2025-09-13T12:50:05.271Z
Learning: NumberRegex pattern `-?[\d\.,]+` in Flow.Launcher Calculator will match function arguments like `2,3` in `min(2,3)`. The NormalizeNumber validation that checks for proper 3-digit thousand separator grouping is essential to avoid breaking function calls by incorrectly stripping commas from function arguments.
Applied to files:
Plugins/Flow.Launcher.Plugin.Calculator/Main.csPlugins/Flow.Launcher.Plugin.Calculator/MainRegexHelper.cs
📚 Learning: 2025-09-13T12:50:05.270Z
Learnt from: dcog989
PR: Flow-Launcher/Flow.Launcher#3971
File: Plugins/Flow.Launcher.Plugin.Calculator/Main.cs:67-67
Timestamp: 2025-09-13T12:50:05.270Z
Learning: In Flow.Launcher Calculator plugin, NormalizeNumber must validate thousand separator usage before stripping them. If a comma-separated token doesn't follow proper thousand separator rules (3 digits between separators), it should be left unmodified to preserve function arguments like in "min(2,3)".
Applied to files:
Plugins/Flow.Launcher.Plugin.Calculator/Main.csPlugins/Flow.Launcher.Plugin.Calculator/MainRegexHelper.cs
🧬 Code graph analysis (2)
Flow.Launcher.Test/Plugins/CalculatorTest.cs (1)
Plugins/Flow.Launcher.Plugin.Calculator/Main.cs (1)
Main(14-431)
Plugins/Flow.Launcher.Plugin.Calculator/Main.cs (1)
Plugins/Flow.Launcher.Plugin.Calculator/MainRegexHelper.cs (1)
MainRegexHelper(5-24)
🪛 GitHub Check: Check Spelling
Flow.Launcher.Test/Plugins/CalculatorTest.cs
[warning] 6-6:
NUnit is not a recognized word. (unrecognized-spelling)
[warning] 7-7:
NUnit is not a recognized word. (unrecognized-spelling)
Plugins/Flow.Launcher.Plugin.Calculator/Main.cs
[warning] 141-141:
nullability is not a recognized word. (unrecognized-spelling)
Plugins/Flow.Launcher.Plugin.Calculator/MainRegexHelper.cs
[warning] 22-22:
isinfty is not a recognized word. (unrecognized-spelling)
[warning] 22-22:
isprime is not a recognized word. (unrecognized-spelling)
[warning] 22-22:
isint is not a recognized word. (unrecognized-spelling)
[warning] 19-19:
bln is not a recognized word. (unrecognized-spelling)
[warning] 13-13:
bpow is not a recognized word. (unrecognized-spelling)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: gitStream.cm
- GitHub Check: build
🔇 Additional comments (3)
Plugins/Flow.Launcher.Plugin.Calculator/MainRegexHelper.cs (1)
13-23: Pow/log/ln/function patterns look solid.Balanced‑group handling + CultureInvariant is appropriate here.
Flow.Launcher.Test/Flow.Launcher.Test.csproj (1)
42-42: Add Calculator plugin reference — LGTM.Helps ensure tests compile against the real plugin.
Plugins/Flow.Launcher.Plugin.Calculator/Main.cs (1)
62-66: Function lower‑casing + early locale capture — LGTM.Good to normalize function names and compute separators once per query.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
Flow.Launcher.Test/Plugins/CalculatorTest.cs (4)
11-13: Guard against parallel test interference with static engineThe test mutates a static field (
MagesEngine). Mark the fixture non‑parallel to avoid cross‑test races.Apply:
- [TestFixture] + [TestFixture] + [NonParallelizable] public class CalculatorPluginTest
1-8: Pin test culture to Invariant for reproducible formatting
FormatResultand grouping can vary by OS culture. Pinning Invariant prevents surprises on CI.Apply:
-using NUnit.Framework.Legacy; +using NUnit.Framework.Legacy; +using System.Globalization; @@ public class CalculatorPluginTest { @@ public CalculatorPluginTest() { @@ } + + [OneTimeSetUp] + public void OneTimeSetupCulture() + { + CultureInfo.DefaultThreadCurrentCulture = CultureInfo.InvariantCulture; + CultureInfo.DefaultThreadCurrentUICulture = CultureInfo.InvariantCulture; + }
74-77: Swap expected/actual in assertion
ClassicAssert.AreEqual(expected, actual)— current order is reversed, leading to confusing failure messages.Apply:
- ClassicAssert.AreEqual(GetCalculationResult(expression), result); + ClassicAssert.AreEqual(result, GetCalculationResult(expression));
40-69: Add regression tests for comma as function-argument separator vs decimal/groupingTo lock in the “strict mode” behavior (don’t strip commas inside functions) and the thousand‑group validation, add a couple of focused tests.
Suggested additions:
Ensure commas inside functions are preserved:
[TestCase(@"min(2,3)", "2")][TestCase(@"pow(min(2,3),3)", "8")]Ensure invalid grouping isn’t stripped in strict mode (from our learning):
- With any function present, tokens like
"1000,000"should remain unchanged and not be misparsed as1000000.
Would you like me to open a follow‑up PR with these tests?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Flow.Launcher.Test/Plugins/CalculatorTest.cs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: dcog989
PR: Flow-Launcher/Flow.Launcher#3971
File: Plugins/Flow.Launcher.Plugin.Calculator/Main.cs:67-67
Timestamp: 2025-09-13T12:50:05.270Z
Learning: In Flow.Launcher Calculator plugin, NormalizeNumber must validate thousand separator usage before stripping them. If a comma-separated token doesn't follow proper thousand separator rules (3 digits between separators), it should be left unmodified to preserve function arguments like in "min(2,3)".
Learnt from: dcog989
PR: Flow-Launcher/Flow.Launcher#3971
File: Plugins/Flow.Launcher.Plugin.Calculator/Main.cs:67-67
Timestamp: 2025-09-13T12:50:05.271Z
Learning: NumberRegex pattern `-?[\d\.,]+` in Flow.Launcher Calculator will match function arguments like `2,3` in `min(2,3)`. The NormalizeNumber validation that checks for proper 3-digit thousand separator grouping is essential to avoid breaking function calls by incorrectly stripping commas from function arguments.
🧬 Code graph analysis (1)
Flow.Launcher.Test/Plugins/CalculatorTest.cs (1)
Plugins/Flow.Launcher.Plugin.Calculator/Main.cs (1)
Main(14-431)
🪛 GitHub Check: Check Spelling
Flow.Launcher.Test/Plugins/CalculatorTest.cs
[warning] 6-6:
NUnit is not a recognized word. (unrecognized-spelling)
[warning] 7-7:
NUnit is not a recognized word. (unrecognized-spelling)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: gitStream.cm
🔇 Additional comments (2)
Flow.Launcher.Test/Plugins/CalculatorTest.cs (2)
31-37: Good: inject custom Mages engine and define eSetting
ein the engine scope aligns with the plugin’s Init path and stabilizes tests involvingln(e).
52-63: Good coverage for pow/log/ln workarounds and common functionsCases for
pow,log,ln, and multi‑arg functions exercise the recent parser/rewriter changes.
|
@Jack251970 I haven't had the chance to look at the code closely, but looks like you guys have done some good changes, happy for you to approve + merge when good. |
Yeah, I will do if this PR is fully tested |
I will add that to my list!
I feel the code now is in a better condition than current production. Plus I think we cannot cover every variable in unit testing, for every language pack, and every possible customisation a user might make and how they might try to use the calculator??? So, LGTM and then react to issues if they are submitted? |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
Flow.Launcher.Test/Plugins/CalculatorTest.cs (3)
60-60: Align expected output for sin(pi)
FormatResultdoes not pad trailing zeros; with rounding it returns"0".-[TestCase(@"sin(pi)", "0.0000000000")] +[TestCase(@"sin(pi)", "0")]
85-89: Fix compile error: resolve Plugin.Query type
Pluginalias isn’t defined; this won’t compile. Use fully-qualified type or add an alias.Apply one of:
- var results = _plugin.Query(new Plugin.Query() + var results = _plugin.Query(new Flow.Launcher.Plugin.Query()or add
using Plugin = Flow.Launcher.Plugin;at the top.
15-20: Make tests locale-stable (freeze culture and decimal separator)Current tests depend on machine culture (UseSystemLocale). Set a deterministic culture and decimal separator to avoid CI flakiness.
Apply:
+using System.Globalization; @@ - DecimalSeparator = DecimalSeparator.UseSystemLocale, + DecimalSeparator = DecimalSeparator.Dot, MaxDecimalPlaces = 10, ShowErrorMessage = false // Make sure we return the empty results when error occurs }; @@ - public CalculatorPluginTest() + private CultureInfo? _originalCulture; + private CultureInfo? _originalUICulture; + + public CalculatorPluginTest() { _plugin = new Main(); @@ engineField.SetValue(null, _engine); } + + [OneTimeSetUp] + public void OneTimeSetUp() + { + _originalCulture = CultureInfo.CurrentCulture; + _originalUICulture = CultureInfo.CurrentUICulture; + CultureInfo.CurrentCulture = CultureInfo.InvariantCulture; + CultureInfo.CurrentUICulture = CultureInfo.InvariantCulture; + } + + [OneTimeTearDown] + public void OneTimeTearDown() + { + if (_originalCulture != null) CultureInfo.CurrentCulture = _originalCulture; + if (_originalUICulture != null) CultureInfo.CurrentUICulture = _originalUICulture; + }To verify determinism, run the test suite twice while temporarily setting a different process culture (e.g., de-DE) and confirm identical outputs.
🧹 Nitpick comments (2)
Flow.Launcher.Test/Plugins/CalculatorTest.cs (2)
29-42: Optionally avoid reflection and static state leakageTests poke private fields and a static engine. Consider adding an injection hook (ctor overload or internal setter with InternalsVisibleTo) and restoring the previous static engine in teardown to prevent cross-fixture bleed.
If preferred, I can draft a small refactor in the plugin to expose safe test-only injection points.
55-67: Add two edge-case tests (nested pow; comma inside functions)Strengthen regressions around rewrites and “comma-as-arg” handling.
// Functions [TestCase(@"pow(2,3)", "8")] + [TestCase(@"pow(2,pow(2,3))", "256")] [TestCase(@"min(1,-1,-2)", "-2")] + [TestCase(@"min(2,3)", "2")] [TestCase(@"max(1,-1,-2)", "1")]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Flow.Launcher.Test/Plugins/CalculatorTest.cs(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: dcog989
PR: Flow-Launcher/Flow.Launcher#3971
File: Plugins/Flow.Launcher.Plugin.Calculator/Main.cs:67-67
Timestamp: 2025-09-13T12:50:05.270Z
Learning: In Flow.Launcher Calculator plugin, NormalizeNumber must validate thousand separator usage before stripping them. If a comma-separated token doesn't follow proper thousand separator rules (3 digits between separators), it should be left unmodified to preserve function arguments like in "min(2,3)".
Learnt from: dcog989
PR: Flow-Launcher/Flow.Launcher#3971
File: Plugins/Flow.Launcher.Plugin.Calculator/Main.cs:67-67
Timestamp: 2025-09-13T12:50:05.271Z
Learning: NumberRegex pattern `-?[\d\.,]+` in Flow.Launcher Calculator will match function arguments like `2,3` in `min(2,3)`. The NormalizeNumber validation that checks for proper 3-digit thousand separator grouping is essential to avoid breaking function calls by incorrectly stripping commas from function arguments.
📚 Learning: 2025-09-13T12:50:05.270Z
Learnt from: dcog989
PR: Flow-Launcher/Flow.Launcher#3971
File: Plugins/Flow.Launcher.Plugin.Calculator/Main.cs:67-67
Timestamp: 2025-09-13T12:50:05.270Z
Learning: In Flow.Launcher Calculator plugin, NormalizeNumber must validate thousand separator usage before stripping them. If a comma-separated token doesn't follow proper thousand separator rules (3 digits between separators), it should be left unmodified to preserve function arguments like in "min(2,3)".
Applied to files:
Flow.Launcher.Test/Plugins/CalculatorTest.cs
🧬 Code graph analysis (1)
Flow.Launcher.Test/Plugins/CalculatorTest.cs (1)
Plugins/Flow.Launcher.Plugin.Calculator/Main.cs (1)
Main(14-431)
🪛 GitHub Check: Check Spelling
Flow.Launcher.Test/Plugins/CalculatorTest.cs
[warning] 6-6:
NUnit is not a recognized word. (unrecognized-spelling)
[warning] 7-7:
NUnit is not a recognized word. (unrecognized-spelling)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (2)
Flow.Launcher.Test/Plugins/CalculatorTest.cs (2)
56-67: Good coverage for function parsing and Mages workaroundsThese cases validate pow/log/ln rewrites and multi-arg functions. Nice.
44-55: Core arithmetic and parentheses cases look solidBaseline scenarios and combined expressions are well covered.
Also applies to: 69-72
My pleasure - even if it made my brain hurt at times!! :) |
Backout 'smart' digit separation & Fix Mages functions for Calculator plugin
Backout 'smart' digit separation & Fix Mages functions for Calculator plugin
Required returning to digit separators obeying user locale setting.
Added workaround for Unexpected
pow(x,y)behavior FlorianRappl/Mages#132Fixes #3750, #3912, #3974