Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Serializing dictionary with ', ", . in key & #37 #38

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

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.

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 #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
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?

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.

None yet

3 participants