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

Multiple type declarations behaviour #641

Closed
DCxDemo opened this issue Oct 25, 2019 · 4 comments
Closed

Multiple type declarations behaviour #641

DCxDemo opened this issue Oct 25, 2019 · 4 comments
Milestone

Comments

@DCxDemo
Copy link

DCxDemo commented Oct 25, 2019

Yesterday I found out I had this silly typo in ksy: I declared 2 different types for the same field. vec2f here is basically 3 floats.

type: vec3f
type: f4

So what happened here is that web-ide used 1st declaration and parsed the whole file correctly (both stable and devel), but C# converted class used second declaration and ruined the whole thing. I tried both stable and latest build from appveyor (labeled c# fixes).

It would be nice to:

  1. have consistent behavior between compiler and web-ide
  2. maybe have a warning of multiple "type" declarations in case this is not the intended behaviour.
@GreyCat
Copy link
Member

GreyCat commented Oct 25, 2019

The problem with this is that part is entirely defined by YAML parser — we don't have much control over that. For some reasons, most YAML parsers treat duplicate keys in a map as normal, picking one of them (i.e. either the first or the last value, based on how the parser is coded). On top of that, we use two different YAML parsers — SnakeYAML for JVM build and YamlJS for JS build, and that can give some inconsistent results too.

There's already #229 to use better, pure Scala YAML parser (may be creating our own) that will work for all Scala targets, but at the moment, this is the best thing we can do.

@GreyCat
Copy link
Member

GreyCat commented Oct 27, 2019

Actually, I went ahead and checked if we can do duplicate keys check at least in some situations, i.e. in SnakeYAML/JVM. It turns out that with recent SnakeYAML versions, we can! So I've added relevant code and a test. Please take a look at newest unstable build?

@GreyCat GreyCat added this to the v0.9 milestone Oct 27, 2019
@DCxDemo
Copy link
Author

DCxDemo commented Oct 27, 2019

Yeah, I'm getting "found duplicate key type" now.

@GreyCat
Copy link
Member

GreyCat commented Oct 27, 2019

Ok, considering this one fixed then, to the extent it is possible now. JS one is a different thing, though :(

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

No branches or pull requests

2 participants