Crystal 0.29.0 support

[elaine-jackson]

Crystal 0.29.0 is out and with breaking changes (https://github.com/crystal-lang/crystal/releases/tag/0.29.0).

@Blacksmoke16 and I discussed over Gitter DMs and we're aware of the following things that needs a fix:

• Json serialization - broken by these changes

[bcardiff]

My understanding is that the .new should be defined in the subclasses, but maybe the initialize(pull: ..) are not. If that's the case something like this might help:

abstract class Granite::Base
  macro inherited
    include JSON::Serializable
    include YAML::Serializable
  end

  def initialize(*, __pull_for_json_serializable pull : ::JSON::PullParser)
    super
  end

  def initialize(*, __context_for_yaml_serializable ctx : YAML::ParseContext, __node_for_yaml_serializable node : ::YAML::Nodes::Node)
    super
  end
end

It seems I need to add this repo to test-ecosystem...

[Blacksmoke16]

@bcardiff I think I have it working where all the required initializers are defined on sub classes, however now I'm getting

no overload matches 'Review#set_attributes' with type Hash(Symbol, YAML::Nodes::Node | YAML::ParseContext)
Overloads are:
 - Review#set_attributes(args : Hash(String | Symbol, Type))
 - Review#set_attributes(result : DB::ResultSet)
 - Review#set_attributes(**args)

    set_attributes(args.to_h)

Where it seems that JSON/YAML serializable types are leaking into the has that gets created via the

  def initialize(**args : Object)
    set_attributes(args.to_h)
  end

initializer.

I haven't looked too much into it yet though.

[bcardiff]

The : Object adds nothing. And maybe is hiding the other initializers. Try directly without restriction. But it might be hitting still some corner case.

  def initialize(**args)
    set_attributes(args.to_h)
  end

also, since there is a set_attributes maybe the serialization code can be changed to use that and it might make more sense in the context of granite. Dunno.

[bcardiff]

FYI granite was added to test-ecosystem crystal-lang/test-ecosystem@47a049f . It would flag issues for the next release earlier. But the stories to how to be up to date with nightlies before release needs to be improved so they can be handled in each shard.

[elaine-jackson]

My understanding is that the .new should be defined in the subclasses, but maybe the initialize(pull: ..) are not. If that's the case something like this might help:

abstract class Granite::Base
  macro inherited
    include JSON::Serializable
    include YAML::Serializable
  end

  def initialize(*, __pull_for_json_serializable pull : ::JSON::PullParser)
    super
  end

  def initialize(*, __context_for_yaml_serializable ctx : YAML::ParseContext, __node_for_yaml_serializable node : ::YAML::Nodes::Node)
    super
  end
end

It seems I need to add this repo to test-ecosystem...

Even with the two initializers inside the inherited macro the specs won't run.

in src/granite/querying.cr:34: instantiating 'DB::ResultSet+#each()'

      results.each do
              ^~~~

in src/granite/querying.cr:35: instantiating 'from_sql(DB::ResultSet+)'

        rows << from_sql(results)
                ^~~~~~~~

in macro '__process_querying' expanded macro: extended:1, line 5:

   1.       
   2.       
   3. 
   4.       # Create the from_sql method
>  5.       disable_granite_docs? def self.from_sql(result)
   6.         model = Validators::PersonUniqueness.new
   7.         model.set_attributes(result)
   8.         model
   9.       end
  10. 
  11.       disable_granite_docs? def set_attributes(result : DB::ResultSet)
  12.         # Loading from DB means existing records.
  13.         @new_record = false
  14.         
  15.           self.id = result.read(Union(Int64 | Nil))
  16.           
  17.         
  18.           self.name = result.read(Union(String | Nil))
  19.           
  20.         
  21.         self
  22.       end
  23.     

expanding macro
in macro 'disable_granite_docs?' /root/Code/granite/src/granite/table.cr:2, line 4:

   1.   
   2.     # :nodoc:
   3.     def self.from_sql(result)
>  4.   model = Validators::PersonUniqueness.new
   5.   model.set_attributes(result)
   6.   model
   7. end
   8.   

no overload matches 'Validators::PersonUniqueness.new'
Overloads are:
 - Validators::PersonUniqueness.new(ctx : YAML::ParseContext, node : YAML::Nodes::Node)
 - Validators::PersonUniqueness.new(pull : ::JSON::PullParser)
 - Validators::PersonUniqueness.new(*, __pull_for_json_serializable pull : ::JSON::PullParser)
 - Validators::PersonUniqueness.new(*, __context_for_yaml_serializable ctx : YAML::ParseContext, __node_for_yaml_serializable node : ::YAML::Nodes::Node)

[Blacksmoke16]

@drujensen @robacarp IMO the easiest solution here is to mark the JSON/YAML specs as pending, remove the JSON/YAML::Serializable modules, then revisit serialization. This would at least allow it to work on 0.29.0, but would obviously break the serialization stuff for people.

As a workaround people could just use JSON.mapping, would have to define each property twice, but at least it would work.

I currently don't have the time to do it myself, however I'd be happy to chat through it/help in the discussion if someone wants to pick it up.

[bcardiff]

@nsuchy that means the argless initializer hidden in the concrete class. Since the last include in the macro inherited section is probably hiding the initializers defined in Base.

I tried moving everything inside the macro inherited hook. And changed the **args to be DB::Any since that is more accurate for the Hash(Symbol | String, DB::Any) overload and would avoid clashing with the json/yaml ones. But... I reached something weirder.

    def initialize(*, __pull_for_json_serializable pull : ::JSON::PullParser)
      super
    end

    def initialize(*, __context_for_yaml_serializable ctx : YAML::ParseContext, __node_for_yaml_serializable node : ::YAML::Nodes::Node)
      super
    end

    def initialize(**args : DB::Any)
      set_attributes(args.to_h)
    end

    def initialize(args : Hash(Symbol | String, DB::Any))
      set_attributes(args)
    end

    def initialize
    end

Error in spec/granite_spec.cr:41: instantiating 'Review.class#from_json(String)'

        review = Review.from_json(json_str)
                        ^~~~~~~~~

in .../from_json.cr:13: read before assignment to local variable ''

  new parser

[elaine-jackson]

@drujensen @robacarp IMO the easiest solution here is to mark the JSON/YAML specs as pending, remove the JSON/YAML::Serializable modules, then revisit serialization. This would at least allow it to work on 0.29.0, but would obviously break the serialization stuff for people.

As a workaround people could just use JSON.mapping, would have to define each property twice, but at least it would work.

I currently don't have the time to do it myself, however I'd be happy to chat through it/help in the discussion if someone wants to pick it up.

Crystal 0.29.0 support is important at the same time I won't want to see functionality go away to get that support.