-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Move to_json and to_yaml to JSON::Serializable and YAML::Serializable #5697
base: master
Are you sure you want to change the base?
Move to_json and to_yaml to JSON::Serializable and YAML::Serializable #5697
Conversation
# | ||
# This macro also declares instance variables of the types given in the mapping. | ||
macro mapping(_properties_, strict = false) | ||
include YAML::Serializable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just in case, I would use ::YAML::Serializable
here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that would be better. Missed it.
f9aa185
to
88a7ab8
Compare
I'd also suggest to remove |
Yes, we can just have |
The specs are failing because 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 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. |
@straight-shoota Always |
I mean, just put This is a problem in Ruby too, everything is so dynamic that depending on what files you 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 {% if constant? "JSON::Serializable" %}
include JSON::Serializable
{% end %} |
1f34e49
to
ff0b2fa
Compare
No, that's not good, because maybe you What we do with, for example, BigInt, is to have a |
Does this need any more work? |
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 Having these methods defined is not really an issue for the actual converter methods The The only problem is the definition of the helper methods So I thought: why not define it always? We could just include dummy modules 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 |
ff0b2fa
to
e19ebd1
Compare
With #6082 introducing I opted for |
e19ebd1
to
d6663c6
Compare
Hi all, sort of relevant. I want to be able to check if I can serialise an object in JSON. 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 @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. 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 |
@stakach ah, so you want a |
In retrospect it probably should have been |
@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 |
@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. |
d6663c6
to
f249e4c
Compare
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 However, while trying to work on this, I faced this issue: #9173. I wonder if this PR would have the same issue if rebased. |
f249e4c
to
5151b10
Compare
This PR adds the modules
JSON::Serializable
andYAML::Serializable
which defineto_json
andto_yaml
methods as wrappers forto_json(JSON::Builder)
andto_yaml(YAML::Nodes::Builder)
.Previously these methods were defined in
Object
which would spill them even to types that don't implement JSON/YAML serialization. Now, they need to be explicitly included.JSON.mapping
andYAML.mapping
include the resepective module automatically. Therefore most user code will most likely not require any changes.If the serialization methods are implemented manually, this will require to add
include JSON::Serializable
/include YAML::Serializable
to the type in order to make the wrapper methods available as well.Closes #5695, #3450