BlackEdder / painlessjson

D library for converting any custom types to and from JSON the painless way.
Boost Software License 1.0
23 stars 3 forks source link

[WIP] fromJSON does not work for range within object #26

Closed BlackEdder closed 9 years ago

BlackEdder commented 9 years ago

@Zalastax would you have any clue why this test fails? From debugging I found that it fails at line 332:

__traits(compiles, mixin ("t." ~ name ~ "= fromJSON!("
                ~ (typeof(__traits(getMember, t, name))).stringof
                ~ ")(jsonAA[\"aFieldName\"])"))

As far as I can tell that should translate into:

t.routes= fromJSON!(Route[])(jsonAA["aFieldName"])

and should compile.

Zalastax commented 9 years ago

I checked what it compiles to (by printing the mixin): t.la= fromJSON!(string)(jsonAA["aFieldName"]) t.routes= fromJSON!(Route[])(jsonAA["aFieldName"])

I will investigate further.

Zalastax commented 9 years ago

I found out why it doesn't compile. I added a static if that would only be true for the case I was searching for. Inside the static if I placed the code I wanted to get the compile errors for. Route isn't available in the scope.

source\painlessjson\painlessjson.d(358): Error: undefined identifier Route
Zalastax commented 9 years ago

So what do you need to do to solve this? For the library user the only available solution right now is to move Route to a public place.
Is there any possibility for the library to do anything different? If we can't support the use case at least add a better error message? If we can't produce a better error message we need to document it clearly.

BlackEdder commented 9 years ago

Thanks @Zalastax for finding that out. I did just try moving the test and moving JourneyPlan to a separate source file and even that did not work. Seems like you would have to move it to a separate library, which seems a big limitation.

About better error: I guess it should be possible to check whether a type is known at compile time.

About fixing it. Not really sure how we could do that. I guess the problem is that the new types are not known yet at compile time. Could we force them to be loaded first? I guess going the mixin approach of jsonizer would work around it. Wonder if cerealed has solved this problem?

Zalastax commented 9 years ago

I think this fix is quite good. The user only have to make their symbols accessible by import. Could you try to fix the errors on older compilers?

BlackEdder commented 9 years ago

Thanks @Zalastax. I guess you still need to define the unittest in a different module, but this is a definitive improvement.

Not sure though if this is fixable on older compilers though. Error looks like a bug in the std library of the older compilers.

Guess could make test etc compiler version specific

BlackEdder commented 9 years ago

Ok, I think this feature is important enough that it warrants dropping support for the older compilers. @Zalastax do you agree? If so I will merge this and bump the version.

Zalastax commented 9 years ago

Yeah. There seems to be substantial bugfixes in the latest released compiler. Before merging you could probably replace https://github.com/BlackEdder/painlessjson/blob/master/source/painlessjson/painlessjson.d#L391-L404 since that bug is fixed in the latest compiler version.