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

Add Hash/Indexable#dig/dig? #6719

Merged
merged 3 commits into from Sep 22, 2018

Conversation

Projects
None yet
6 participants
@Sija
Contributor

Sija commented Sep 14, 2018

Followup to #3536.

@j8r

This comment has been minimized.

Contributor

j8r commented Sep 14, 2018

It would be also great to have #dig(keys : Enumerable), where keys represents a variable sequence of keys.

@RX14

This comment has been minimized.

Member

RX14 commented Sep 14, 2018

Thought we decided against this. It was .dig or JSON::Any and JSON::Any won because my proposal forgot that JSON::Type included more than Hash and Array. I proposed it could be solved by making JSON.parse return Hash | Array because the 95% usecase for json is arrays or hashes at the top level, but that idea wasn't well recieved.

@j8r that method would just produce insane types.

@Sija

This comment has been minimized.

Contributor

Sija commented Sep 14, 2018

@RX14 I understood differently, also according to your own #3536 (comment).
@j8r That would forgo all compile-time safety, ditto what @RX14 said.

I still see it as a very handy addition to stdlib and see no reason not to implement it.

@Sija Sija force-pushed the Sija:dig-it branch from de7558e to 2266945 Sep 14, 2018

@asterite

This comment has been minimized.

Contributor

asterite commented Sep 14, 2018

I was against dig because I thought the implementation would make the return type be the union of everything. But this implementation is brilliant.

a = {1 => [[1, 2], [3, 4]]}

p typeof(a.dig(1, 0)) # => Array(Int32)
p a.dig(1, 0) # => [1, 2]

p typeof(a.dig(1, 0, 1)) # => Int32
p a.dig(1, 0, 1) # => 2

So I see it as a very nice, convenient way, to traverse deep structures. Now we would just need to add it to JSON::Any and YAML::Any... or remove those ugly Any wrappers altogether (I'd start by adding dig/dig? to those types, and eventually decide how to remove them).

@Sija

This comment has been minimized.

Contributor

Sija commented Sep 14, 2018

So I see it as a very nice, convenient way, to traverse deep structures.

Exactly.

... or remove those ugly Any wrappers altogether (I'd start by adding dig/dig? to those types, and eventually decide how to remove them).

Yes, please! 🎉

@asterite

This comment has been minimized.

Contributor

asterite commented Sep 14, 2018

@Sija Do you want to add dig/dig? to Any then? At least that way this PR will be "complete".

@Sija

This comment has been minimized.

Contributor

Sija commented Sep 14, 2018

@asterite Sure, will do.

@codenoid

This comment has been minimized.

Contributor

codenoid commented Sep 14, 2018

hi @Sija , long time no c

@Sija

This comment has been minimized.

Contributor

Sija commented Sep 14, 2018

@asterite Done.
@codenoid Indeed, good to c u back! :)

@Sija Sija force-pushed the Sija:dig-it branch from 18c38ce to 0143933 Sep 14, 2018

@Sija

This comment has been minimized.

Contributor

Sija commented Sep 17, 2018

Is it GTG now? CI fails seems not related, btw.

@Sija

This comment has been minimized.

Contributor

Sija commented Sep 22, 2018

Hello? Is this got stalled for a reason?

@RX14

RX14 approved these changes Sep 22, 2018

@bcardiff

GTG on my side. Maybe NamedTuple should also have #dig to traverse them directly.

@Sija

This comment has been minimized.

Contributor

Sija commented Sep 22, 2018

@bcardiff I like it 👍, it's here in the last commit.

@bcardiff

This comment has been minimized.

Member

bcardiff commented Sep 22, 2018

@Sija CircleCI had a bad week in their servers.

In darwin is still failing because it trying to compile the compiler with llvm 7, which is still not supported. The dependency of the formula is llvm plain and that leads to llvm 7 right now.

@RX14 RX14 added this to the 0.27.0 milestone Sep 22, 2018

@RX14 RX14 merged commit 19338f0 into crystal-lang:master Sep 22, 2018

3 of 4 checks passed

ci/circleci: test_darwin Your tests failed on CircleCI
Details
ci/circleci: test_linux Your tests passed on CircleCI!
Details
ci/circleci: test_linux32 Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

ezrast pushed a commit to ezrast/crystal that referenced this pull request Oct 2, 2018

Add Hash/Indexable#dig/dig? (crystal-lang#6719)
* Add Hash/Indexable#dig/dig?

* Add JSON/YAML::Any#dig/dig?

* Add NamedTuple#dig/dig?

omarroth added a commit to omarroth/crystal that referenced this pull request Nov 5, 2018

Add Hash/Indexable#dig/dig? (crystal-lang#6719)
* Add Hash/Indexable#dig/dig?

* Add JSON/YAML::Any#dig/dig?

* Add NamedTuple#dig/dig?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment