Skip to content
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

Struct serialize to json #13

Closed
wants to merge 5 commits into from

Conversation

krzykamil
Copy link
Contributor

https://github.com/orgs/hanami/projects/6/views/1?pane=issue&itemId=70528802

This just adds a simple json conversion to struct attributes. Dumps to hash and then json.

I saw the issue description mentioned that it could also be moved to ROM or DRY gems, but I found two discussion that convinced me against it:

dry-rb/dry-struct#166 here Nikita said "Serializing to JSON is not part of the gem and I'm against mixing concerns in general." and in the ROM thread, there was a similar mention, by Piotr rom-rb/rom#403 "In general I would recommend encapsulating json serialization/deserialization with your own APIs, instead of referring to 3rd party gems"

So i thought its best not to nest this method too deep in those gems

@cllns
Copy link
Member

cllns commented Sep 23, 2024

@krzykamil Can you take another look at this and fix the CI failures? They seem straight-forward to fix, happy to take it over if needed.

@krzykamil
Copy link
Contributor Author

@cllns my bad, completely forgot about this and the rubocop. Should be fixed now

@cllns
Copy link
Member

cllns commented Oct 9, 2024

Thanks @krzykamil! Do you mind changing it so that the args are passed through to the underlying .to_json? I don't think people use it much, but we might as well use the args instead of dropping them.

@krzykamil
Copy link
Contributor Author

Thanks @krzykamil! Do you mind changing it so that the args are passed through to the underlying .to_json? I don't think people use it much, but we might as well use the args instead of dropping them.

I'll do it over the weekend, no problem

@alassek
Copy link
Contributor

alassek commented Oct 18, 2024

Merged as ec2942f

@alassek alassek closed this Oct 18, 2024
@alassek
Copy link
Contributor

alassek commented Oct 18, 2024

Thanks for the contribution, @krzykamil! I did some cleanup history rewriting on the commits so GitHub doesn't see it as a merge, but you're still listed as the author.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants