I'd also suggest to remove to_pretty_json. This could become a parameter on to_json. I don't think it makes sense to keep these duplicated methods.

Yes, we can just have indent: "" and to have a pretty json you'd just pass ident: " ".

The specs are failing because Crystal::Exception defines to_json(JSON::Builder) and includes JSON::Serializable. But in some of the sample files for individual compiler features, json is not required and therefore we have an unknown constant.
Other locations in the compiler itself assume an Crystal::Exception to have a to_json method when json is available.

What would be the best way to handle this?

I guess that's not just an issue for the compiler itself, but also for libraries which support serialization but can be used without it, if the application doesn't need it. In that case, you can't just always include Serializable.

One solution could be to include the serialization modules in the core lib. But that sounds very wrong.

This very hacky solution would make it work using a macro to check if JSON is available:

{% if @type.has_constant? :JSON %}
module Crystal
  abstract class Exception
    include JSON::Serializable
  end
end
{% end %}

But I guess this could become somewhat reasonable if it was possible to check for the existance of global namespace constants directly in the type namespace. But I don't think that's currently possible.
Therefore the macro needs to be put in the global namespace (so @type references the program) and reopen the class to add the include.

@straight-shoota Always require "json" in the compiler and tests for the compiler.

I mean, just put require "json" where include JSON::Serializable is needed.

This is a problem in Ruby too, everything is so dynamic that depending on what files you require things work or not (in this case compile or not).

The solution is to have an import system like Go or Python, where to use something you have to import it, every time. It's a bit more tedious, but more robust. But I don't think we'll ever do that in Crystal.

For the compiler and compiler samples, this is probably okay.

But I think it would be feasible to have a more general working solution to easily switch the include statement on or off depending on the existance of a global constant like this:

{% if constant? "JSON::Serializable" %}
include JSON::Serializable
{% end %}

No, that's not good, because maybe you require "json" after requiring that piece of code, and by then it's too late.

What we do with, for example, BigInt, is to have a big/json file that you can require to add JSON support to those types. For the compiler I don't think it's worth it, though.

Does this need any more work?

@asterite

I'm not satisfied with having to require shards differently depending on whether I want to use them with support for JSON and/or YAML serialization or not.

This concerns shards or stdlib components which define data types that support serialization but don't depend on it. The shard can be used without JSON, YAML but in case you want to serialize them, the methods for that should be there and if possible don't require anything more than adding require "json" to be available.

Having these methods defined is not really an issue for the actual converter methods to_json(builder : JSON::Builder) and new(parser : JSON::PullParser) (and equivalent for YAML). These methods can always be defined in the source regardless of JSON usage. They won't be parsed unless they are called. It's just easily pluggable: If you're not using json it shouldn't be an issue having these methods. If you want to use json, they should just work without ado.

The mapping macros are not really affected, they need JSON/YAML always be available anyway. Considering this is a very tight integration of the serialization format in the type, this shouldn't be an issue. In the long term we'll probably end up with something different anyway (see #3620).

The only problem is the definition of the helper methods to_json() and to_json(io : IO) defined in JSON::Serializable (with this PR). It shouldn't be required to redefine these methods on every such type supporting pluggable serialization. Unfortunately, differently from the methods which will not get parsed, include JSON::Serializable can't be used in a type definition if that module is not defined.

So I thought: why not define it always? We could just include dummy modules JSON::Serializable and YAML::Serializable in the core lib. They can be included but don't define anything:

module YAML::Serializable
end

Maybe as an usability impovement they could have the serialization helper methods defined but raise and prompt to require the respective library.

module JSON::Serializable
  def to_json
    {% raise %(missing JSON, please add `require "json"`) %}
  end
end

This solution surely isn't optimal as it add's type definition (even if they are empty) to the core lib which shouldn't be needed to be there. It obviously only works for serialization formats included in the stdlib (although I think JSON and YAML should stay in stdlib) as this won't be a general solution for other serialization formats as well.

But the suggested solution improves ease of use by a great deal and I think that's very important - IMO the benefit should be worth it and I don't see any downsides that would make this impossible.

FYI This now conflicts with #6082 but is undetected in the diff

Yeah, I didn't have time to properly look at #6082 yet and see what from this PR needs to be adapted.

Oh, I didn't mean to close this. Can someone reopen, please?

Reopened. I was only pointing that it was not ok to merge. :-) Not that I was hurry or had any specific input in this PR

With #6082 introducing {JSON,YAML}::Serializable to automatically expose instance variables for serialization, the methods added in this PR should go into a different module in order to separate helper methods like #to_json from autogeneration of to_json(builder).

I opted for {JSON::YAML}::Serializable::Helper, though I'm not sure about that. It a quite long name for being frequently included and add's another included module to JSON::Serializable namespace. Are there any suggestions to rename?

Hi all, sort of relevant. I want to be able to check if I can serialise an object in JSON.
i.e. result.responds_to?(:to_json) however this always returns true as it's defined on Object.

It would be good to be able to check if an object can be serialized - also not sure if there is a solution to that problem I could use at the moment?

@stakach there's no solution right now. What's the usecase?

you can use hack https://play.crystal-lang.org/#/r/5ysu

Thanks for the hint @kostya
With some slight modifications I have the desired result: https://play.crystal-lang.org/#/r/5yx3

@RX14 we build automation / workplace management systems https://www.acaprojects.com/ and are moving our platform to Crystal from Ruby. We have drivers for hardware (example ruby drivers) and the public methods of a driver class become the API for that driver.
We return null if the result of a function execution can't be serialised as the result is often not that important however sometimes the result is desirable.

The crystal version of this is pretty cool, macros introspect the user defined class to auto-generate a public API. Each device type gets their own process and we support live updating of the production systems by compiling new versions when updates occur and re-launching the individual processes. https://github.com/aca-labs/crystal-engine-driver

@straight-shoota Could you rebase this PR against master? I think it's a good idea. Maybe instead of Helper we can name it Base. In any case it's not a super important name because it's mostly used in the std, most types will directly use JSON::Serializable.

@stakach ah, so you want a to_json if it exists or nil. That's a good usecase, thanks!

In retrospect it probably should have been JSON::Serializable for this interface and JSON::Mapper for the macro module.

@RX14 Why retrospect? We can still change this.

It's still retrospective introspection even if we can still change it. I'd support changing it. I'd like to hear other's opinions though, since it's a breaking change over "just" naming.

maybe instead of this, add my hack as option to method responds_to? (to check by params types). This was not in ruby, but in crystal it possible.

@kostya sounds like a good addition, given there could be multiple method signatures. Still worth doing this though

@kostya that's more of an orthogonal issue. There might already be an issue for that, if not please make one.

Yesterday I came up with a similar idea and @asterite brought me to #6275 and this PR. I think this is actually good and should be merged eventually. Would it be possible actually to use the same JSON::Serializable module and make it generate or not the to_json(JSON::Builder) depending on wether is already defined? I was playing with JSON::Friendly but not a good name anyway 😆

However, while trying to work on this, I faced this issue: #9173. I wonder if this PR would have the same issue if rebased.