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

Add path function #157

Merged
merged 3 commits into from
Apr 18, 2023
Merged

Add path function #157

merged 3 commits into from
Apr 18, 2023

Conversation

cuihtlauac
Copy link

Allows recurisve access to nested object fields using a list of keys

Copy link
Member

@Leonidas-from-XIV Leonidas-from-XIV left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest

val path : string list -> t -> t option

Which makes the semantics of path clear for when the segments don't lead to an existing value.

lib/util.mli Outdated
the JSON object [obj], or [`Null] if [l] did no led to an object in
[obj]. [path [k1; ...; kn] obj] is the same as
[member kn (... (member k1 obj))]
@raise Type_error if [obj] is not a JSON object. *)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think in 2022 we shouldn't be adding more unsafe functions that throw exceptions. It would be better to return t option here and I am somewhat unsure that returning `Null is a behavior that makes sense in OCaml. If I saw a function like this I would not expect path ["missing"] (Assoc [])to return ``Null``, I would rather expect it to throw Not_found following the stdlib.

(I am aware that it uses member for which the exact same concerns apply which unfairly gets a free pass for being like this since forever)

Cuihtlauac ALVARADO and others added 3 commits April 18, 2023 10:45
Allows recurisve access to nested object fields using a list of keys
@Leonidas-from-XIV
Copy link
Member

@panglesd Can you take a look at this PR and weight in whether you think this is reasonable? It's a bit unusual as the API is bit more exception-safe than the rest.

Copy link
Collaborator

@panglesd panglesd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are now in 2023 and I think we should even less add unsafe API than in 2022.
Consistency with the rest of the API seems, to me, less important than having safer functions.

The string list -> t -> t option and its implementation look good to me.

@Leonidas-from-XIV Leonidas-from-XIV merged commit 6cb37fd into ocaml-community:master Apr 18, 2023
Leonidas-from-XIV added a commit to Leonidas-from-XIV/opam-repository that referenced this pull request Apr 26, 2023
CHANGES:

*2023-04-26*

### Added

- Add `Yojson.Raw.Util` module to provide combinators for extracting fields
  from `Yojson.Raw.t` values. (@tmcgilchrist, ocaml-community/yojson#163)

- Add `Util.path` function to recurse into an object through a list of keys.
  (@cuihtlauac, @Leonidas-from-XIV, ocaml-community/yojson#157)
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