fix: Fixes grammar and implementation test mismatch for alignments #6
Conversation
While implementing a message templates parser I pulled the message templates tests from the C# repo to ensure that my parsing behaviour was correct & translated them into rust. While I was doing that I saw that alignment was specified to be able to be any digits preceeded by an optional '-' but the test "ZeroValuesAlignmentIsParsedAsText". Since this seems to be the most common behaviour across the ecosystem and there's a specific test for it I expected that this was an oversight in the spec.
|
Thanks for this, it's good to try to nail this down. Serilog (where the majority of these tests came from :-)) originally intended to mirror the behavior of .NET's format string syntax, where It'd be interesting to see what the consequences of making this case valid in Serilog might be. I don't think we'd be against falling into line with NLog and .NET format strings there, if there was no added execution time or complexity cost 🤔 |
|
Thanks for responding! Yeah I think the only other one that doesn't seem to jive with the spec from what I'm seeing in the serilog repo is: APropertyWithValidNameAndInvalidFormatIsParsedAsText because the format grammar is I think it might be good as I was kind of alluding to in messagetemplates/test-cases#5 to have larger/more standard corpus... and ideally a test runner that can kick off a process, input a bunch of test cases into its stdin and evaluate the output from stdout so that we can make it easy to check impls in other langs. I'm still cleaning up my impl, I've been standardizing on two error behaviours and that'll probably be the last step and I can toss it into the test-cases repo. Thinking something like: |
|
@nblumhardt re:
Want me to take a look at this then? |
|
Hi! Sounds great; I can't tell at this point whether it's something we'd end up wanting to change, but some investigation would be really helpful 👍 |
|
I've made the change and benched it in serilog/serilog#2064 |
While implementing a message templates parser I pulled the message templates tests from the C# repo to ensure that my parsing behaviour was correct & translated them into rust. While I was doing that I saw that alignment was specified to be able to be any digits preceded by an optional '-' but the test ZeroValuesAlignmentIsParsedAsText indicates that the first digit should not be able to be zero.
This seems to be the intended behaviour & there isn't a consensus across the ecosystem: Your repo + serilog treat it as invalid, though I believe serilog just nabbed most of your tests and nlog treats it as valid. However given there's a specific test for it I expected that this was an oversight in the grammar definition and figured I could help either fix it here or in the tests.