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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allowing JSON.mapping & YAML.mapping converter attribute to be applied to Array and Hashes. #8156

Merged

Conversation

@rodrigopinto
Copy link
Contributor

rodrigopinto commented Sep 5, 2019

What is this PR for?

This PR is for making JSON.mapping & YAML.mapping support a converter to be applied to an Array or a Hash.

馃憠 Closes #7981

src/json/to_json.cr Show resolved Hide resolved
@rodrigopinto rodrigopinto force-pushed the rodrigopinto:rp-json-mapping-custom-converters branch 2 times, most recently from 6891bcc to 1b08cd4 Sep 12, 2019
@RX14
RX14 approved these changes Sep 18, 2019
@RX14

This comment has been minimized.

Copy link
Member

RX14 commented Sep 18, 2019

This looks really good! If you don't have the time to implement YAML::HashConverter, this can be merged now since it's a strict improvement.

@rodrigopinto

This comment has been minimized.

Copy link
Contributor Author

rodrigopinto commented Sep 19, 2019

@RX14 Thanks for the review. I actually started it already, I hadn't enough time last week to push it forward. Hope to get it on ready for final review this week.

@rodrigopinto rodrigopinto force-pushed the rodrigopinto:rp-json-mapping-custom-converters branch from 1b08cd4 to a81d498 Sep 19, 2019
@rodrigopinto rodrigopinto marked this pull request as ready for review Sep 19, 2019
@rodrigopinto

This comment has been minimized.

Copy link
Contributor Author

rodrigopinto commented Sep 19, 2019

@RX14 @straight-shoota I finished the implementation of the YAML converters, it is ready for another review. I did some squash but a few "independent commits", let me know if more squashes are needed.

node.raise "Expected mapping, not #{node.class}"
end

hash = Hash(String, typeof(Converter.from_yaml(ctx, node))).new

This comment has been minimized.

Copy link
@Sija

Sija Sep 19, 2019

Contributor

(unlike JSON) YAML allows keys of different types, not just String.

This comment has been minimized.

Copy link
@RX14

RX14 Sep 19, 2019

Member

JSON allows this too, on the crystal types side. This can be improved later with a different converter if you want to convert the keys too.

This comment has been minimized.

Copy link
@rodrigopinto

rodrigopinto Sep 20, 2019

Author Contributor

@Sija, that is true, we can improve it. I do believe that it could take another PR for that. Considering Hash also allows union types for the Key, the converter would need to handle the cast possibilities of different types, or would we make it strict?

This comment has been minimized.

Copy link
@straight-shoota

straight-shoota Sep 20, 2019

Member

Both Hash converters simply need two generic arguments, one for converting the key, the other for converting the value. Just like Hash has two generic arguments as well.

This comment has been minimized.

Copy link
@straight-shoota

straight-shoota Sep 20, 2019

Member

And I'd rather have this included here to avoid breaking the API later.

This comment has been minimized.

Copy link
@rodrigopinto

rodrigopinto Sep 24, 2019

Author Contributor

So, I played a bit with a possible YAML::HashConverter, but wind up with some points.

Different from the basic types Int32, String, Time, etc..., the Time::EpochConverter, Time::EpochMillisConverter and others custom converters are modules which expose the method self.from_yaml that is used from the macro mapping here.

So, to make a Hash works with union types, for example, (Time::EpochConverter | String) would be better to refactor these custom converters to have similar API of the basic types.

For example:

struct Time::EpochConverter
  def from_yaml(ctx : YAML::ParseContext, node : YAML::Nodes::Node) : Time
    unless node.is_a?(YAML::Nodes::Scalar)
      node.raise "Expected scalar, not #{node.class}"
    end

    Time.unix(node.value.to_i)
  end
end

So, with that, we would avoid the situation commented on the snippet below.

module YAML::HashConverter(K, V)
  def self.from_yaml(ctx : YAML::ParseContext, node : YAML::Nodes::Node)
    unless node.is_a?(YAML::Nodes::Mapping)
      node.raise "Expected mapping, not #{node.class}"
    end

    hash = Hash(K, V).new

    YAML::Schema::Core.each(node) do |key, value|
      # here on the V.from_yaml(ctx, value)
      # It would work for if the type is Time::EpochConverter.from_yaml(ctx, value)
      # but in case it is a String, would be required something like String.new(ctx, value) instead
      hash[K.new(ctx, key)] = V.from_yaml(ctx, value)
    end

    hash
  end
end

Does it make sense @straight-shoota? Not sure if I missed something, so feel free to point out if I took the wrong path.

Concluding, I am happy to continue evolving this PR until we agree it is in a good shape, or either get it merged and open another PR with the changes + a YAML::HashConverter for keys and values.

wdyt @RX14 ?

This comment has been minimized.

Copy link
@RX14

RX14 Sep 27, 2019

Member

String.from_yaml should have the same shape as a converter - this is how it works for JSON.

Please make an issue about this. Would you be okay leaving the YAML stuff off before that is refactored? What would others prefer?

This comment has been minimized.

Copy link
@rodrigopinto

rodrigopinto Oct 8, 2019

Author Contributor

@RX14 I think the better to keep the YAML::HashConverter outside of this PR and we proceed the merge with the others.

I think we can keep the YAML::ArrayConverter and I will continue the YAML::HashConverter in another PR, but I want to avoid this one to become "statel".

This comment has been minimized.

Copy link
@RX14

RX14 Oct 17, 2019

Member

Yes, please remove the hash converter and we can merge.

This comment has been minimized.

Copy link
@rodrigopinto

rodrigopinto Oct 20, 2019

Author Contributor

@RX14 done. I will start another one soon with tha yaml hash converter.

@rodrigopinto rodrigopinto force-pushed the rodrigopinto:rp-json-mapping-custom-converters branch from a81d498 to 69059fb Oct 19, 2019
@RX14
RX14 approved these changes Oct 29, 2019
@bcardiff bcardiff merged commit 5e7e2f6 into crystal-lang:master Nov 8, 2019
5 of 6 checks passed
5 of 6 checks passed
ci/circleci: test_linux Your tests failed on CircleCI
Details
ci/circleci: check_format Your tests passed on CircleCI!
Details
ci/circleci: test_darwin Your tests passed on CircleCI!
Details
ci/circleci: test_linux32_std Your tests passed on CircleCI!
Details
ci/circleci: test_preview_mt Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bcardiff

This comment has been minimized.

Copy link
Member

bcardiff commented Nov 8, 2019

This is going to be used so much 馃殌
Thanks @rodrigopinto !

didactic-drunk added a commit to didactic-drunk/crystal that referenced this pull request Nov 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can鈥檛 perform that action at this time.