Skip to content

Conversation

@spaceone
Copy link
Contributor

@spaceone spaceone commented Sep 24, 2025

Fixes: #28

Depends on #49 and #50

@spaceone spaceone mentioned this pull request Sep 24, 2025
@spaceone spaceone force-pushed the feat/parser branch 4 times, most recently from e7ee688 to 59f95bc Compare September 24, 2025 11:16
@td-anne
Copy link

td-anne commented Sep 24, 2025

It would be good to have a parametrized round-trip test - that is, take a (valid) dict of strings to strings, logfmt it, then parse_logfmt, and check that the dict you get back is the dict you put in. One could then construct variously unpleasant dictionaries and verify that they came through all right. One could also add a check that invalid dictionaries that didn't make sense in logfmt (empty string as key?) raise a sensible exception.

@spaceone
Copy link
Contributor Author

My test already compares the format with the parsed result.

@td-anne
Copy link

td-anne commented Sep 24, 2025

My test already compares the format with the parsed result.

Indeed. But round-trip tests are a verification of both parts of the system, and are much easier to construct difficult examples for.

@spaceone
Copy link
Contributor Author

I can easily split the tests case and make the round trip:
assert parse_logfmt(Logfmter.format_params(expected), **kwargs) == expected

but I currently already also verify the exact resulting string.
And then I need at least 2 test cases - because there are many cases which are valid but aren't the canonical form logfmter would produce. E.g. at="INFO" is equally valid to at=INFO.

@spaceone
Copy link
Contributor Author

@td-anne I added two more additional round trip tests

@josheppinette
Copy link
Owner

@spaceone once this is merged with latest (and conflicts resolved), it should pass the tests (since newline update has been merged)

@josheppinette
Copy link
Owner

And then I need at least 2 test cases - because there are many cases which are valid but aren't the canonical form logfmter would produce. E.g. at="INFO" is equally valid to at=INFO.

If we really wanted this, then we would probably have some normalization before comparing the strings, but I don't think its necessary (at least for initial rollout).

@spaceone
Copy link
Contributor Author

spaceone commented Sep 24, 2025

@josheppinette I rebased it. But I don't think it's complete. We should speak about some behavior:

  1. Should we allow ' single quotes? logfmt only produces double quotes. I disabled them for now. go-logfmt declined a PR to add single quote support as well.
  2. Strings can come from user input, so they are arbitrary. How would you expect a = b to be parsed?
$ echo 'a = b' | ./external/golang-logfmt-echo/golang-logfmt-echo 
error: logfmt syntax error at pos 3 on line 1: unexpected '='

I can't easily detect this case.

  1. The current implementation doesn't put nested keys back into a dictionary structure. But doing so, is unsafe.
  2. maybe more things to consider.

@spaceone
Copy link
Contributor Author

  1. I am not sure about compatibility with unquoted backslash sequences. The external compatibility test fails due to my demo change in 72e9060.
    For example: logfmt currently produces:
>>> logfmter.Logfmter.format_params({'a': '\\', 'b': 'c'})
'a=\\ b=c'

Similar to the newline issue, I would have expected:

>>> logfmter.Logfmter.format_params({'a': '\\', 'b': 'c'})
'a="\\\\" b=c'

Would you consider this change as valid?

@spaceone
Copy link
Contributor Author

$ echo 'foo="\\" bar=baz' | ./golang-logfmt-echo 
foo=\ bar=baz

@spaceone
Copy link
Contributor Author

go-logfmt/logfmt#16

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add Parser

3 participants