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

Cleanup YAML/JSON Any#dig methods #9415

Merged
merged 4 commits into from
Jul 30, 2020

Conversation

j8r
Copy link
Contributor

@j8r j8r commented Jun 3, 2020

Any#[] always return an Any, so it is always diggable.
This also means the exception is superfluous.

The logic inside the methods is also simplified, and return types added.

@j8r j8r changed the title Cleanup json yaml any dig Cleanup YAML/JSON Any#dig methods Jun 3, 2020
@bcardiff
Copy link
Member

bcardiff commented Jun 4, 2020

Do you mind applying the same to datum?

@j8r
Copy link
Contributor Author

j8r commented Jun 4, 2020

Interesting this Crystal.datum - why it is not used in JSON::Any and YAML::Any?

@bcardiff
Copy link
Member

bcardiff commented Jun 4, 2020

Internal and I wanted to avoid early generalization. It was used in log's config and context. But config was never merged in the end.

@j8r
Copy link
Contributor Author

j8r commented Jun 4, 2020

We can let this as-is for now then.

All tests pass, except the darwin test was canceled; don't know why.

Note: I cannot view Circle CI logs anymore, it redirects to https://circleci.com/vcs-authorize/ (?!) :(

@j8r j8r force-pushed the cleanup-json-yaml-any-dig branch from c5d210a to 7a409a0 Compare July 29, 2020 14:02
@j8r
Copy link
Contributor Author

j8r commented Jul 29, 2020

Is it possible to move forward on this?

@bcardiff bcardiff added this to the 1.0.0 milestone Jul 29, 2020
@bcardiff bcardiff merged commit 8de7f2d into crystal-lang:master Jul 30, 2020
@j8r j8r deleted the cleanup-json-yaml-any-dig branch July 30, 2020 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants