Skip to content

Conversation

ITR13
Copy link
Contributor

@ITR13 ITR13 commented Jan 14, 2024

Fixes issue where GetTopLevelAndSubKeys splits a key when serializing.
Adds test to handle new cases.
Does not test cases with mixed " and ' due to issue explained in #37.

ITR13 added 2 commits January 14, 2024 03:30
Fixes issue where GetTopLevelAndSubKeys splits a key when serializing.
Adds test to handle new cases.
Does not test cases with mixed " and ' due to issue explained in SamboyCoding#57
@coveralls
Copy link

coveralls commented Jan 14, 2024

Coverage Status

coverage: 92.835% (+0.9%) from 91.909%
when pulling 9061871 on ITR13:master
into acbaa75 on SamboyCoding:master.

@ITR13 ITR13 marked this pull request as draft January 14, 2024 21:26
Ideally fixes SamboyCoding#37, but current implementation has some issues.
Will elaborate on issues with this "fix" in a comment.
@ITR13 ITR13 changed the title fix: Serializing dictionary with ', ", . in key. fix: Serializing dictionary with ', ", . in key & #37 Jan 14, 2024
@ITR13
Copy link
Contributor Author

ITR13 commented Jan 14, 2024

Easiest way to understand the issue is the test StringTests.EscapedQuotesInAKeyAreValid:
The key string used there is "\"a.b\"", which when passed into StringTests.EscapedQuotesInAKeyAreValid would correctly only create a single key
The problem is that the parser pre-unescapes the key to "a.b", which breaks the whole thing. It might be possible to have it not unescape it, and instead unescape it later, but I don't know it this breaks other code.

The parser is also unable to parse strings in the form "a.b".c or ""a.b"".c, which both are allowed in the toml specification.
For that I can imagine two possible fixes, but I haven't looked at the parser yet and would not know of any other issues:

  • Make sure it doesn't look only for full quoted keys, then send the whole string back unmodified
  • Add proper splitting & escaping at parser-level, then remove GetTopLevelAndSubKeys completely and instead have keys be handled as some sort of list of strings passed into ContainsKey and GetValue.
    I haven't worked enough with text processing to know the performance or memory usage impacts of either solution though.

@ITR13 ITR13 marked this pull request as ready for review January 20, 2024 02:27
@ITR13 ITR13 marked this pull request as draft January 20, 2024 02:32
ITR13 added 7 commits January 20, 2024 03:34
Added test for redefining dotted key.
Made InternalPutValue always have lineNumber since it's used by the parser.
Deleted now unused exception.
The current exception thrown here already shows line-number.
I think rider had a field-day when getting to generate Designer.rs
Added tests for String Reader since Backtrack wasn't covered.
Added test for Document.ToString()
Added test for more places it's possible to reach the end of the file.
Added test for different ways of writing numbers.
@ITR13 ITR13 marked this pull request as ready for review January 20, 2024 03:42
@levicki
Copy link

levicki commented Mar 15, 2024

This fix seems to be related to the issue I reported. Is there any ETA for it?

@ITR13 ITR13 marked this pull request as draft February 26, 2025 13:13
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.

3 participants