Repository navigation
Conversation
benPearce1
reviewed
Sep 18, 2026
benPearce1
left a comment
Contributor
There was a problem hiding this comment.
This looks good, but needs some extra thought might be the error states
Comment on lines
+22
to
+23
| if (argument == null || options.Length != 1) | ||
| return null; |
Contributor
There was a problem hiding this comment.
This should probably return a formatted error
| return null; | ||
|
|
||
| if (!TryParseNumber(argument, out var left) || !TryParseNumber(options[0], out var right)) | ||
| return null; |
Contributor
There was a problem hiding this comment.
This feels like it should just return false, or an error
Adds LessThan, LessThanOrEqual, GreaterThan and GreaterThanOrEqual, so a numeric comparison can be used anywhere a variable expression can be - including as a run condition on its own, which is what #114 asked for. Implemented as filters rather than as new grammar. `#{if <expression>}` already accepts a full filter chain, so this covers run conditions, conditionals and `#{elseif}` chains without touching the parser, and without changing how any existing template is parsed. Both sides are parsed with the invariant culture so that a template gives the same answer wherever it is evaluated. NumberStyles.Float excludes thousands separators, so an ambiguous value like "1,000" is treated as non-numeric rather than being read as 1. A value that is not numeric returns null, which leaves the token unevaluated and is not truthy - matching the existing text comparison filters. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Addresses review feedback on the error states. A value that is present but not a number now compares as false, rather than leaving the expression unevaluated: no number can satisfy the comparison, so false is the honest answer. The wrong number of arguments is a mistake in the template itself that no value could make valid, so it now returns Error.Format, consistent with UriPart. The caller is captured with CallerMemberName so the message names the filter the author wrote rather than the private helper. An unresolved variable still returns null and is reported as a missing token, consistent with every other filter. Returning false there would make a misspelled variable name quietly compare as false instead of being reported. Keeping false (rather than a formatted error) for a non-numeric value matters because Error.Format strings are truthy - IsTruthy only treats "", "0", "no" and "false" as falsy - so returning an error from a comparison would make a broken run condition evaluate true and run the step. All three failure modes are non-truthy. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
droyad
force-pushed
the
robw/issue-114-comparison-operators
branch
from
September 18, 2026 05:04
9e55c4f to
af08732
Compare
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.
Adds
LessThan,LessThanOrEqual,GreaterThanandGreaterThanOrEqualfilters, so a numeric comparison can be used anywhere a variable expression can be — including as a run condition on its own.That replaces the workaround in the issue, which was to
#{calc {X} - 31}into a variable and then test it with| StartsWith "-".Why filters rather than new syntax
The issue proposed three syntaxes with no stated preference: bare
#{X < 31}, a| LessThan 31filter, or#{calc {X} < 31}. The filter form wins on more than just cost:#{if <expression>}already accepts a full filter chain, so a single filter covers run conditions, conditionals and#{elseif}chains at once:It also means no grammar change at all —
TemplateParser.csis untouched, so there is no way for this to alter how any existing template containing<or>is parsed. Given that Octostache parses every variable in every Octopus deployment, that seemed worth a lot. It composes with other filters (#{Total | Trim | GreaterThan 100}) for free, and leaves the door open to adding the bare#{X < 31}form later as sugar over the same logic.#{calc {X} < 31}was rejected on design grounds:calc's entire type model isdouble?, and a comparison yields a boolean.Semantics
Each decision follows an existing precedent in the codebase:
"true"/"false"StartsWith,EndsWith,Contains,Matchnull→ token left unevaluated, recorded as missing, not truthyCalculationVariable.EvaluatenullStartsWithwith 0 or 2+ optionsReturning
"true"/"false"keeps these consistent with the existing text-comparison filters and withIsTruthy, so they behave correctly as run conditions.Number parsing is invariant-culture.
double.TryParse(value, NumberStyles.Float, CultureInfo.InvariantCulture)— a template gives the same answer regardless of the culture of the machine evaluating it, rather than a run condition that can flip depending on which worker picks it up.NumberStyles.Floatdeliberately excludes thousands separators, so an ambiguous"1,000"is treated as non-numeric instead of being silently read as1.Note this is deliberately not consistent with
#{calc}, which parses and formats using the current culture in four places. Nothing in the Octostache readme or the public docs specifies a number format forcalc, so there was no documented contract to match, and I did not want to propagate machine-dependent behaviour into something new.calcis left alone in this PR.NaN and Infinity parse and then follow ordinary
doublecomparison semantics (soNaN | LessThan 31andNaN | GreaterThanOrEqual 31are bothfalse). Pinned by a test rather than left to chance.Out of scope
AND/ORoperators, requested in a comment on the issue. Not blocked by this — they would be additive, as either filters or grammar.#{X < 31}and#{calc {X} < 31}forms.calc's culture handling.Testing
TDD: tests written first and watched fail, then the implementation. Full suite is green on both target frameworks — 696 passed, 0 failed on
net10.0andnet48— which is the backwards-compatibility safety net for a library this widely exercised.Coverage includes each operator's boundary behaviour (
5 | LessThan 5false vs5 | LessThanOrEqual 5true), numeric-not-lexicographic ordering (9 | LessThan 10), negatives, decimals, exponents, surrounding whitespace, non-numeric on either side, wrong arity, variable options, filter chaining, case-insensitive filter names, and the invariant-culture decision pinned via"1,5"and"1,000". Integration tests cover the issue's run condition verbatim (indexed output variable and all), a missing/misspelled variable evaluating to false rather than silently passing, and comparisons inside#{if}and#{elseif}chains.Docs
README.mdgains a short "Comparison filters" section. The public docs atoctopus.com/docs/projects/variables/variable-filterswill need a matching entry under Comparison filters — separate repo, not done here.Resolves #114
🤖 Generated with Claude Code