Add JSON.def_to_json #4772

straight-shoota · 2017-08-01T11:51:30Z

This puts the def to_json generator from JSON.mapping in a separate macro to make it individually accessible where automatically declared instance variables and parser are not needed. This comes in two flavors: one for converting self to JSON (this is the same as JSON.mapping without the extra features) and one which accepts the object-to-be-converted as an argument - this is useful to define JSON converters.
Further documentation and specs are included.

This PR was extracted from #4746 which also shows an exemplary usecase.

straight-shoota · 2017-08-01T12:40:40Z

src/json/def_to_json.cr

+      json.object do
+        {% for key, options in mappings %}
+          {% unless options.is_a?(HashLiteral) || options.is_a?(NamedTupleLiteral) %}
+            {% options = {nil: nil} %}


This looks ugly but I couldn't find a way to define an empty NamedTupleLiteral. Suggestions are welcome.

@straight-shoota NamedTuple.new.

@RX14 won't work: https://carc.in/#/r/2iyw, @straight-shoota wants to assign a literal, so it can be used as a container later in the macro

straight-shoota · 2017-08-01T17:12:43Z

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 ? =)

makenowjust · 2017-08-06T14:56:35Z

@straight-shoota done in #4801.

straight-shoota · 2017-08-11T12:18:58Z

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?

RX14 · 2017-08-11T17:11:07Z

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.

straight-shoota · 2017-08-11T22:10:15Z

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.

bew · 2017-08-14T10:19:17Z

src/json/def_to_json.cr

+  # whose keys will define JSON properties.
+  #
+  # The value of each key can either be `true` or a hash or named tuple literal with the following options:
+  # * **property**: the property name on the Crystal object (as opposed to the key in the in the JSON document)


typo: in the in the

bew · 2017-08-14T10:31:38Z

src/json/def_to_json.cr

+  end
+
+  # :nodoc:
+  macro field_to_json(value_name, field_name, options)


this macro name and its arguments are inconsistent:

field_to_json: you want to generate the code that will transform the crystal-field to json. So here field == crystal-field.

value_name: wut?

field_name: from the macro name, we could understand that it's the crystal-field name. In fact it's the json-key name.

I think that to keep things coherent you can either:

Change the macro name to value_to_json

Change the argument field_name to key or json_key, and the argument value_name to field or field_name.

I guess I did this a bit sloppy because it is only an internal macro and was refactored quite a few times... ;)

But I like your suggestions, thanks for reviewing!

I think I'll change the macro name to emit_value_to_json - it's better to have a verb.

I am not sure about field_name because it is technically not always referring to a field but might as well be an ordinary method returning a value unrelated to an instance variable.

I think I'll change the macro name to emit_value_to_json - it's better to have a verb.

right, it's good too!

I am not sure about field_name because it is technically not always referring to a field but might as well be an ordinary method returning a value unrelated to an instance variable.

True, then maybe getter_name ? but that's not ok with @var accessor. or just accessor_name?

As explained below it does not necessarily have to refer to a getter or any method on self but may in fact be any expression. So maybe better just use value_expression?

bew · 2017-08-14T10:32:58Z

src/json/def_to_json.cr

+    ::JSON.def_to_json({{type}}, {{mappings}})
+  end
+
+  # The `StringConverter` has a class method `to_json` wich can be used as a converter for `JSON.def_to_json`. The value is added


typo: wich => which

bew · 2017-08-14T10:45:07Z

src/json/def_to_json.cr

+  # whose keys will define JSON properties.
+  #
+  # The value of each key can either be `true` or a hash or named tuple literal with the following options:
+  # * **property**: the property name on the Crystal object (as opposed to the key in the JSON document)


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: { property: name.downcase } }) # to_json will output {"name":"foobar"}

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

I guess this feature should also be noted in the documentation...

Hmm I'm not sure I completely agree, but it makes sense, and given this concept differences I think it's acceptable (and interesting to have too!).

Also I think you should copy/paste this discussion (only the important bits: question & answer?) in the issue's conversation, so your explaination about the differences of concept can't get lost as a line comment when/if you change this particular line of code.

bew · 2017-08-14T12:26:41Z

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}

straight-shoota · 2017-08-14T18:08:26Z

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.

straight-shoota · 2017-08-14T18:18:12Z

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.

straight-shoota · 2017-08-14T19:02:58Z

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

bew · 2017-08-14T23:11:49Z

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!

straight-shoota · 2017-08-15T10:09:33Z

@bew That looks like a great idea!

straight-shoota · 2017-10-11T10:06:18Z

Does this PR need any additional work?

…tailed instructions for removal

RX14

Overall I think the documentation needs to be improved, it's dry and at times misleading.

RX14 · 2017-10-31T14:36:49Z

src/json/def_to_json.cr

+module JSON
+  # The `def_to_json` macro generates a `to_json(json : JSON::Builder)` method based on provided *mappings*.
+  #
+  # It is a lightweight alternative to `JSON.mapping` if you don't need to declare instance variables and a parser.


I'd phrase this in terms of "serialization-only" or "read-only" instead of "if you don't need to declare instance variables and a parser" because your docs don't get near the heart of what def_to_json is used for.

RX14 · 2017-10-31T14:38:24Z

src/json/def_to_json.cr

+  #
+  # It is a lightweight alternative to `JSON.mapping` if you don't need to declare instance variables and a parser.
+  #
+  # The generated method invoks `to_json(JSON::Builder)` on each of the values returned by the *value* expression,


what "the value expression" is is undefined before this point.

RX14 · 2017-10-31T14:40:04Z

src/json/def_to_json.cr

+  # require "json"
+  #
+  # record Location, lat : Float64, long : Float64 do
+  #   JSON.def_to_json([lat, long])


Where is this array stuff defined?

Oh, I forgot to change this. The array syntax was not really useful.

RX14 · 2017-10-31T14:44:18Z

src/json/def_to_json.cr

+  # * **converter**: specify an alternate type for generation. The converter must define `to_json(value, JSON::Builder)` as class methods. Examples of converters are `Time::Format` and `Time::EpochConverter` for `Time`.
+  # * **root**: assume the value is inside a JSON object with a given key
+  #
+  # If it is not a hash or named tuple literal, the expression will be interpreted as `value` parameter. As a shortcut `_` represents


I'm not sure about the _ shortcut, I think foo: foo is perhaps the clearest. I can see it getting terrible with long names though.

asterite · 2017-10-31T14:56:38Z

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.

RX14 · 2017-10-31T15:01:45Z

@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

straight-shoota · 2017-10-31T15:05:42Z

@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

asterite · 2017-10-31T15:08:04Z

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.

asterite · 2017-10-31T15:10:12Z

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.

straight-shoota · 2017-10-31T15:10:53Z

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.

RX14 · 2017-10-31T15:14:29Z

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.

straight-shoota · 2017-10-31T15:15:30Z

Okay, I'll try this for #4746

straight-shoota · 2018-06-13T08:39:17Z

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

straight-shoota commented Aug 1, 2017

View reviewed changes

straight-shoota added a commit to straight-shoota/crystal that referenced this pull request Aug 1, 2017

Rebase to use JSON.def_to_json from crystal-lang#4772

6ead851

straight-shoota added a commit to straight-shoota/crystal that referenced this pull request Aug 1, 2017

Rebase to use JSON.def_to_json from crystal-lang#4772

64ade24

straight-shoota mentioned this pull request Aug 1, 2017

JSON output for docs #4746

Merged

straight-shoota force-pushed the jm-def-to-json branch from 2ca0946 to a6e9b93 Compare August 1, 2017 16:14

straight-shoota added a commit to straight-shoota/crystal that referenced this pull request Aug 1, 2017

Rebase to use JSON.def_to_json from crystal-lang#4772

13da880

straight-shoota mentioned this pull request Aug 6, 2017

Parser: prevent to finish a macro body when {% if/for/begin %} is nested #4801

Merged

straight-shoota force-pushed the jm-def-to-json branch 2 times, most recently from 4e585fa to 693482b Compare August 11, 2017 11:53

straight-shoota force-pushed the jm-def-to-json branch from 693482b to 6cf8f50 Compare August 11, 2017 21:48

straight-shoota force-pushed the jm-def-to-json branch from 6cf8f50 to dd1c264 Compare August 14, 2017 09:55

bew reviewed Aug 14, 2017

View reviewed changes

straight-shoota mentioned this pull request Aug 17, 2017

How to distinguish absence of key-value pairs vs null values in JSON.mapping? #4840

Closed

straight-shoota added 2 commits October 31, 2017 15:13

Add JSON.def_to_json

ab70d7f

Fix pollution from not-so-private records

d9d1219

straight-shoota added 6 commits October 31, 2017 15:13

Improved workaround for crystal-lang#4769 to make formatter happy, de…

b5c59d5

…tailed instructions for removal

Fixed typos (thanks @bew)

3e575ac

Rename JSON.field_to_json to JSON.emit_value_to_json

1924997

Added documentation and spec for arbitrary expressions as property

3cccfb4

Renamed option property to value

bfb980d

Added shortcut syntax for value and special expression _

ff40766

straight-shoota force-pushed the jm-def-to-json branch from fb76538 to ff40766 Compare October 31, 2017 14:15

RX14 requested changes Oct 31, 2017

View reviewed changes

Reformat JSON.def_to_json specs to group example classes and specs

4e791f5

Improve documentation

0359366

straight-shoota added a commit to straight-shoota/crystal that referenced this pull request Oct 31, 2017

Rebase to use JSON.def_to_json from crystal-lang#4772

4c63b44

straight-shoota closed this Jun 13, 2018

straight-shoota deleted the jm-def-to-json branch June 13, 2018 08:39

Add JSON.def_to_json #4772

Add JSON.def_to_json #4772

Conversation

straight-shoota commented Aug 1, 2017 • edited Loading

straight-shoota Aug 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bew Aug 14, 2017 • edited Loading

Choose a reason for hiding this comment

straight-shoota commented Aug 1, 2017 • edited Loading

makenowjust commented Aug 6, 2017

straight-shoota commented Aug 11, 2017

RX14 commented Aug 11, 2017 • edited Loading

straight-shoota commented Aug 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

straight-shoota Aug 14, 2017 • edited Loading

Choose a reason for hiding this comment

straight-shoota Aug 14, 2017 • edited Loading

Choose a reason for hiding this comment

bew Aug 14, 2017 • edited Loading

Choose a reason for hiding this comment

bew commented Aug 14, 2017

straight-shoota commented Aug 14, 2017

straight-shoota commented Aug 14, 2017 • edited Loading

straight-shoota commented Aug 14, 2017

bew commented Aug 14, 2017

straight-shoota commented Aug 15, 2017

straight-shoota commented Oct 11, 2017

RX14 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asterite commented Oct 31, 2017

RX14 commented Oct 31, 2017

straight-shoota commented Oct 31, 2017 • edited by asterite Loading

asterite commented Oct 31, 2017

asterite commented Oct 31, 2017

straight-shoota commented Oct 31, 2017

RX14 commented Oct 31, 2017

straight-shoota commented Oct 31, 2017

straight-shoota commented Jun 13, 2018

straight-shoota commented Aug 1, 2017 •

edited

Loading

straight-shoota Aug 1, 2017 •

edited

Loading

bew Aug 14, 2017 •

edited

Loading

straight-shoota commented Aug 1, 2017 •

edited

Loading

RX14 commented Aug 11, 2017 •

edited

Loading

straight-shoota Aug 14, 2017 •

edited

Loading

straight-shoota Aug 14, 2017 •

edited

Loading

bew Aug 14, 2017 •

edited

Loading

straight-shoota commented Aug 14, 2017 •

edited

Loading

straight-shoota commented Oct 31, 2017 •

edited by asterite

Loading