CI checks are failing because of a formatter issue. I couldn't isolate the reason for this but it looks like it might be related to #4769 - at least it occurs at a similar location, inside field_to_json. I'd be happy if someone could take a look at this! Maybe @makenowjust ? =)

@straight-shoota done in #4801.

Rebased on master with #4801. Builds are still failing because the last commit removes the workaround and the default compiler (0.23.1) does not have the fix from #4801. The compiler needs to be built from current master. I guess this won't work without nightly builds. Should I keep the workaround in for now?

@RX14 What do you think about this PR in general?

I don't understand the point of this addition, or how it relates to JSON.mapping.

Also, all releases must be buildable by the last release. Nobody is going to merge this before it passes CI.

Alright, so I'll keep the workaround until the next compiler version is released.

JSON.mapping consists of three individual tasks all three are based on the fields defined in properties.

• define instance variables
• define an initializer to read object state from a JSON::PullParser
• define a to_json method to export object state with a JSON::Builder

For some usecases (like exporting the compiler's type and method definitions to json) there is no need to define ivars nor a parser which would both come with JSON.mapping.
So currently, you have to define a plain to_json method manually. Such methods have very repetitive code and can easily abstracted. It is nice to not have to write the same stuff over and over again but have a simple macro which creates such a to_json method. An additional benefit of this macro is, that the behaviour is no longer expressed in code but simple declarative data (a named tuple) similar to properties of JSON.mapping.

This functionality was for the most part already implemented in JSON.mapping but it could not be used individually without the ivars and parser. So I extracted this part of JSON.mapping into a macro JSON.def_to_json which is employed by JSON.mapping but can also be used directly.

I would suggest value instead of property, as like you pointed out earlier it's not always a property of the object, it can be a method call!
So an example would be:

@name = "FooBar"
JSON.def_to_json({
  name: { value: name.downcase }
})

And to simplify a bit: as in JSON.mapping, where name: Type is the same as name: {type: Type}, we could say that name: name.downcase is the same as name: {value: name.downcase}

I've thought about that simplification. It's just that then it's difficult to have a shortcut for the case that JSON key and property name are equal. This is currently encoded by the value true (in fact it could be anything that is not a named tuple or hash literal). If the value is used as value expression by default, you'd have to have certain exceptions like that it does not apply if it is a NamedTupleLiteral or HashLiteral (because they used to hold options) and perhaps BoolLiteral or NilLiteral as well as it could be used as a shortcut for equal names.
For the typical usecase I was thinking of (like #4746) it would be mostly simple mappings with identical key and property names. But having a shortcut to define property (or value) option is also quite powerful.
I am honestly not sure which way is better.

Copied from line comments by @bew (question) and me (answer) so it doesn't get lost:

In the current JSON.mapping macro, one specify the crystal's property name as key in the properties arg (your mappings arg), then you can specify an optional key if the JSON field name is different.

Here you did it the other way around, why?

I think it makes more sense this way: with def_to_json you're basically describing a JSON object schema. The keys of this schema correspond to the keys of the named tuple and the value defines which value is to be assigned.
mapping on the other hand has a different focus: it primarily defines properties as instance variables and adds methods to convert them to JSON and back. Here it makes sense to have the crystal properties as keys (because that's the main part) and the JSON field names as arguments.
While the properties argument looks quite similar, both macros have a different usecase and allow different options, so I don't think it is bad to have different semantics between key and value.

For def_to_json there is also the benefit that this way it is very easy to use arbitrary expressions as value:

@name = "FooBar"
JSON.def_to_json({
  name: { value: name.downcase }
})
# to_json will output {"name":"foobar"}

This would not be possible or look very nasty if it was used as a key.

@RX14 there have been a few CI builds where only linux64 failed with fork: Cannot allocate memory (Errno).

• https://travis-ci.org/crystal-lang/crystal/jobs/264329947
• https://travis-ci.org/crystal-lang/crystal/jobs/264463818
• https://travis-ci.org/crystal-lang/crystal/jobs/264300926

About the simplification, here an example of what you could use:

JSON.def_to_json({
  element: _, # Use element
  node: hello.node_getter, # Use hello.node_getter
  stuff: _, # Use stuff
})

Example: https://carc.in/#/r/2j3c

I find it pretty readable, with no duplication, and this could allow the simplification I talked about earlier!

@bew That looks like a great idea!

Does this PR need any additional work?

Just a note that a more granular API isn't always good. Look at Java, for example. Many ways to do the same things is also confusing. I don't understand why JSON.mapping is not enough. If it generates a initialize(pull_parser) method that you don't need, it's not that terrible, really. Now users will have JSON.def_to_json and JSON.mapping to choose. There's also this weird location: _ syntax.

I am personally not sure about this.

@asterite this is for the case where you don't want to serialize all the instance variables, only a subset, so the generated initialize would be invalid

@asterite The main problem with JSON.mapping for serialize-only is not an unnecessary initialize method, it's the instance variables. As evidenced by the usages of this macro in #4746, most of the JSON values are not stored in ivars but are actually return values from methods.
If you wanted to achieve the same, you'd have to create dedicated export types with JSON.mapping for each exportable type

For Crystal::Doc::Method this would look like this:

def to_json(builder : JSON::Builder)
  MethodExport.new(self).to_json(builder)
end

struct MethodExport
  JSON.mapping(
      id: String,
      html_id: String,
      name: String,
      doc: String,
      summary: String,
      abstract: String,
      args: String,
      args_string: String,
      source_link: String,
      def: String
    )
  def initialize(method)
    @id = method.id
    @html_id = method.html_id
    @name = method.name
    # ...
  end
end

You can always do:

  def to_json(builder : JSON::Builder)
    builder.object do
      builder.field "name", @name
      builder.field "age", @age
    end
  end

There's no need to create an extra object with the mapping.

I'm still not sure introducing a huge refactor and more public API methods is good in this case.

Also, once we introduce meta attributes for instance variables, all of this will be kind of obsolete. You'll just specify which instance variables should be serialized, and how. That will work for generating JSON and for parsing, like in every other programming language. So I wouldn't continue making changes to JSON just yet.

Yes, manually writing to a JSON::Builder is possible, but produces a lot of duplicate boilerplate code. It's certainly debateable if this is worth it. But in fact, most of the functionality is already there in JSON.mapping, this macro just exposes this to use it independently. So there is only few additional code added by this PR.

Personally I don't mind the manual to_json using JSON::Builder. It's certainly not too much extra boilerplate. It's still 1 line per ivar.

Okay, I'll try this for #4746

I don't think this is worth pursuing anymore, given that building the JSON manually is pretty straightforward.