flyx / NimYAML

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

Fixes for Nim v0.20.0 #69

Closed sthenic closed 5 years ago

sthenic commented 5 years ago

I thought I'd take a stab at making the library compile with the newly released v0.20.0.

My fork is here: https://github.com/sthenic/NimYAML

Apart from a fair number of warnings (probably need to fix those too) there are two breaking changes:

  1. Macro arguments of type typedesc are now passed to the macro as NimNode like every other type except static. Use typed for a behavior that is identical in new and old Nim. See the RFC Pass typedesc as NimNode to macros for more details. (nim-lang/RFCs#148)

  2. case object branch transitions via system.reset are deprecated. Compile your code with -d:nimOldCaseObjects for a transition period.

There's some additional information in the commit messages but I believe I've handled (1) properly, opting to pass a NimNode to the procs that were previously passed a typedesc.

I would really like some help in dealing with (2) though. Here is the Nim issue tracking the change. The section in the manual has also been updated to describe the new behavior.

Compiling with -d:nimOldCaseObjects works and adding that flag throughout the test suite will make it compile and run all tests successfully, so there's that.

However, I couldn't figure out how to make the necessary changes to remove the need to have the flag. I see that system.reset is called in two places in serialization.nim and I guess the main goal would have to be to introduce some other scheme to initialize/change the variant objects.

This is were I felt it went from patching the code to actually changing the structure. I'm not that experienced with macros yet either so any help would be appreciated.

k0zmo commented 5 years ago

I made myself a fork with necessary changes to build NimYAML with v0.20 without the --define:nimOldCaseObjects. All tests build and run successfully.

The bad news are I couldn't figure out how to make it work with variant objects that have more than one case statement:

  AnimalKind = enum
    akCat, akDog
  DumbEnum = enum
    deA, deB, deC, deD
  Animal = object
    name: string
    case kind: AnimalKind
    of akCat:
      purringIntensity: int
    of akDog: barkometer: int
    case anotherKind: DumbEnum
    of deA, deB: 
      dumbValue: int
    else: 
      anotherDumbValue: string

I'm not sure how much this is popular or is it considered a good practice. Personally, I haven't yet came across such objects so I turned the serialization generation of them to a compile-time error.

sthenic commented 5 years ago

So what's the next step? Open a pull request with the changes from your fork?

I was hoping that @flyx would weigh in at some point.

I'm guessing #70 is related to the new Nim version but not something that's covered by the test suite?

flyx commented 5 years ago

With PR #71 merged, this is fixed.

RokkuCode commented 5 years ago

installing the yaml lib via nimble i stumbled over the error that "queues" module is not found. i had to install it with "yaml@#head". maybe you should make a new release so users are able to install the right version with the fixes for nim-0.20.