OctopusDeploy / Octostache

| Public | The variable substitution syntax for Octopus Deploy
Other
5 stars 24 forks source link

Enable multiple spaces before pipe symbol #82

Closed ghost closed 2 years ago

ghost commented 2 years ago

This PR resolves https://github.com/OctopusDeploy/Issues/issues/7613 where when more than one spaces appears before the pipe symbol (|) in filters expression, the expression cannot be parsed correctly. For example,

[Fact]
public void MultipleSpacesTest()
{
    var result = Evaluate("#{Foo  | ToUpper}", new Dictionary<string, string> { { "Foo", "Abc" } });
    result.Should().Be("ABC");
}

will fail (note there are two spaces before |) because the actually parsed symbol is Foo (with a trailing space) rather than Foo. If you define the value for Foo instead, it can pass.

mjhilton commented 2 years ago

Initially, I wasn't sure about this change because it seemed like we were dramatically loosening the syntax, and I wasn't sure whether that was a good idea or not.

I've had a run of the new test you added without the code change to the parser, and it looks like all but one of these scenarios already passed:

✅ [InlineData("#{Foo|ToUpper}")]
✅ [InlineData("#{Foo | ToUpper}")]
❌ [InlineData("#{Foo  | ToUpper}")]
✅ [InlineData("#{Foo |  ToUpper}")]
✅ [InlineData("#{ Foo | ToUpper}")]
✅ [InlineData("#{  Foo | ToUpper}")]
✅ [InlineData("#{Foo | ToUpper }")]
✅ [InlineData("#{Foo | ToUpper  }")]

This gives me confidence that we're not making a material/possibly-breaking change to the behaviour, just covering off a case that probably should work compared to the rest of the parsing.

The only question remaining for me would be: how does this behave for other things like the each keyword, and are there similar tests (and/or code changes) we should add for those to keep it all consistent?

ghost commented 2 years ago

Thanks for the feedback. I've checked the cases for if and each:

Based on these, I think we can enable leading spaces for each so that the behaviour is consistent, and add tests for those supported but not yet tested. The tricky part IMO is to enable multiple spaces before in, == and !=. Currently only single space is recognised before them. When there are multiple spaces, the rest will be treated as part of the preceding variable name (e.g. a instead of a). This will need a bit more time to fix properly. But since this issue is not urgent I think that's fine.