flyx / NimYAML

YAML implementation for Nim
https://nimyaml.org
Other
191 stars 36 forks source link

Fix code for v0.19.0 #64

Closed Vindaar closed 6 years ago

Vindaar commented 6 years ago

First of all, this PR is based on @alehander42 PR to fix the nil strings.

Besides this, there's been several changes which also cause regressions in the test cases. From the top of my head:

And as it seems converting an integer, which doesn't fit into some int type throws a different error? The case that tries it fails, so I hacked that.

With these changes the test suite works up to Load tuple - missing field. There it still crashes. I'll try to see what's going on there/

Maybe (some) of the changes I made are stupid ways to handle the stuff? Quite possibly, since I don't know the code base. The hardest part for me is the internal exception handling, since I can't see where the ACTUAL exception is being raised, since it's replaced by the "user focused" stack trace, which simply points to the YAML call in the user file and a note "please report the bug".

Vindaar commented 6 years ago

Thanks! I'll fix your requested changes tomorrow. I mainly still need to figure out why some tests are still broken though. :)

Vindaar commented 6 years ago

On my machine there's only a single test case, which fails. Will fix that too.

I also had to fix one proc in the lexer, which also depended on accessing the null terminator of a string. The solution is pretty verbose and maybe you can come up with a nicer (more scalable) solution for that?

And I'll also address the last two remaining comments of yours ~tomorrow.

Vindaar commented 6 years ago

@flyx Ok, so I've addressed all your comments. Two questions:

  1. I'm not sure if my extension of Level in tojson is quite correct. I'm resetting the expKey field of the tuple to true, if the key is being set to "", which I assume is not correct. I guess setting an empty key shouldn't always indicate that a key is expected afterwards.
  2. with the changes now all tests but [S98Z] pass. The expected output of that is the empty string, but we recover the \n and the comment. I guess I introduced that regression due to the changes I made to deal with the null terminator not being accessible. My understanding of yaml and this library isn't quite suited to debug this. Any hint on where I should start?

edit: interestingly enough there's this issue: https://github.com/yaml/yaml-test-suite/issues/37

Vindaar commented 6 years ago

Yup, thanks. I also prefer to not have to change the lexer. I just don't understand the code enough to know where to introduce the \0 by myself. I'll take a look at StringSource creation tomorrow and change the lexer back! :) Any idea regarding the broken test case?

flyx commented 6 years ago

Any idea regarding the broken test case?

That's not a regression, just checked with nimyaml.org testing ground; it also fails with current code. You can put it in the list of ignored tests for now to make the tests green. That list is kind-of a TODO list for lexer problems. I also have doubts whether the test case is correct in the first place; anyway it's an edge case and not relevant for general NimYAML usability. I might evaluate the spec productions to check whether this test is correct sometime.

Vindaar commented 6 years ago

Ok, so I addressed all your comments. Lexer is reverted back to its original form and instead we just append a null terminator to the source.

One thing about that though: at first I only added a null terminator to the StringSource.src. But to avoid having to duplicate the same in the JS branch I instead defined a sourceNull at the beginning of the newYamlLexer proc. That should mean we copy the source before assigning it to the StringSource though (needed for JS as far as I understand, strings being immutable there, but unnecessary for C). Is that ok?

Vindaar commented 6 years ago

Done. :)