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

Some fixes #38

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Some fixes #38

wants to merge 1 commit into from

Conversation

Yardanico
Copy link
Contributor

@Yardanico Yardanico commented Jul 29, 2020

Used normal object copy instead of deepCopy since TomlValueRef of kind Date, Time and DateTime don't contain any ref types inside.
For the removal of type checking in arrays - as per https://github.com/toml-lang/toml/blob/master/toml.md#user-content-array arrays can now be of mixed types.

Also added handling for floats with exponents with leading zeroes

@Yardanico
Copy link
Contributor Author

Another thing I might add - there are more-or-less updates tests in https://github.com/iarna/toml-spec-tests

@PMunch
Copy link
Member

PMunch commented Aug 3, 2020

Hmm, mixed type arrays are not part of the 0.5.0 version of the spec, which is what this is currently up to date with. Could you split your fixes from this feature and submit this as two separate requests so that the 0.5.0 version can be correct according to the spec and we can start working on the 1.0.0 version of the spec separately.

And floats with exponents having leading zeroes is explicitly disallowed by the specification (both the v0.5.0 and the v1.0.0.):
In the float part: "An exponent part is an E (upper or lower case) followed by an integer part (which follows the same rules as integer values)."
In the integer part: "Leading zeros are not allowed."
So this should continue to be a parsing error.

@PMunch
Copy link
Member

PMunch commented Aug 3, 2020

Good thing there is a new test-suite. We really should make a proper one though, that covers 1.0.0 and commit to actually updating it..

@Yardanico
Copy link
Contributor Author

@PMunch I updated the test suite in another branch in my fork, but not all tests pass :) They require more changes to parsing

@kaushalmodi
Copy link
Member

@Yardanico If you rebase this PR to the current parsetoml master branch and force push to the same PR branch, it will resolve the travis error we are seeing right now. That issue got fixed in 3760867 .

kaushalmodi pushed a commit to kaushalmodi-forks/parsetoml that referenced this pull request Jun 7, 2021
.. since TomlValueRef of kind Date, Time and DateTime don't contain
any ref types inside.

Ref: NimParsers#38
kaushalmodi pushed a commit that referenced this pull request Jun 7, 2021
.. since TomlValueRef of kind Date, Time and DateTime don't contain
any ref types inside.

Ref: #38
@kaushalmodi
Copy link
Member

@Yardanico @PMunch I have merged just the "Used normal object copy instead of deepCopy since TomlValueRef of kind Date, Time and DateTime don't contain any ref types inside." piece of this PR in #47 .

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