Crystal: Include type restriction on getters in JSON/mapping

Created on 5 Apr 2018  路  4Comments  路  Source: crystal-lang/crystal

System: Ubuntu WSL, Crystal v0.24.2

JSON#mapping doesn't specify the restriction on getter methods in the macro.

This causes an issue with macros where the Return Type cannot be inferred from a method call.

Minimal code to reproduce:

require "json"

macro method_return(method_name)
  puts "{{method_name}} {{My_Class.methods.find { |method| method.name == method_name}.return_type}}"
end

class My_Class
  JSON.mapping(mapped: Bool)
end

method_return(:mapped)

Output:

:mapped 

I have a working solution here which just replaces #L83 with
def {{key.id}} : {{value[:type]}} {{ (value[:nilable] ? "?" : "").id }
to include the type. Will just wait for approval before Specs & PR

This could also be approached by changing #return_type to be able to infer which would include some more situations as shown below.

Extra code that demonstrates current behaviour:

require "json"

macro method_return(method_name)
  puts "{{method_name}} {{My_Class.methods.find { |method| method.name == method_name}.return_type}}"
end

class My_Class
  JSON.mapping(mapped: Bool)

  def restricted : Int64
  end

  def inferrable
    "string"
  end

  def not_inferrable
  end
end

method_return(:mapped)
method_return(:restricted)
method_return(:inferrable)
method_return(:not_inferrable)

Output:

:mapped 
:restricted Int64
:inferrable 
:not_inferrable 
help-wanted feature stdlib

Most helpful comment

I'm not sure about your use case, but adding the return type explicitly sounds like a good idea. It is known when the macro is interpreted, so there is no point in letting the global inference figure it out later. The setter method already has it, it's just missing for the getter.
It also improves the API docs when the return type is explicit.

All 4 comments

I'm not sure about your use case, but adding the return type explicitly sounds like a good idea. It is known when the macro is interpreted, so there is no point in letting the global inference figure it out later. The setter method already has it, it's just missing for the getter.
It also improves the API docs when the return type is explicit.

Sorry, this is my use-case

require "json"

struct Storable(T)
  JSON.mapping(
    data: {type: T, converter: DataConverter}
  )

  def initialize(@data : T)
  end
end

class My_Class
  JSON.mapping(
    return: My_Return_Class
  )
end

class My_Return_Class
end

module DataConverter
  macro json_convert(klass, **build)
    def self.to_json(instance : {{klass}}, builder : JSON::Builder)
      builder.object do
        {% if build[:fields] %}
          {% for field in build[:fields] %}
            object = instance.{{field.id}}
            unless object.is_a?(Nil)
              builder.field({{field}}, Storable({{klass.resolve.methods.find{ |method| method.name == field}.return_type}}).new(object).to_json)
            end
          {% end %}
        {% end %}
      end
    end
  end

  json_convert(My_Class, fields: {:return})
end

Eh, you can just let the compiler figure out the generic type argument: https://carc.in/#/r/3u0z

builder.field({{field}}, Storable.new(object).to_json)

Fixed via #5935

Was this page helpful?
0 / 5 - 0 ratings

Related issues

asterite picture asterite  路  3Comments

ArthurZ picture ArthurZ  路  3Comments

grosser picture grosser  路  3Comments

will picture will  路  3Comments

lgphp picture lgphp  路  3Comments