Skip to content

Conversation

Shark-with-Blue-Shoes
Copy link

This is the solution to #198. I made a alternative to the member function that raises an error Missing_Member if it can't find a member.

@Leonidas-from-XIV
Copy link
Member

Thanks for the PR. However, I don't think this is the right approach, for a number of reasons:

  1. We should not introduce more exceptions into the code base. It makes reasoning about the code harder as everything can throw an exception and you have to guard in a lot of places and if you forget, the program fails at runtime
  2. The exception has a whole message in English. But one really only wants to know the name of the key, the meaning of the exception is already provided by the name of the exception.
  3. The usual naming for this would probably more member_opt (as in "option"), as these days we'd probably use an option type to indicate the absence of something.
  4. val path : string list -> Yojson.Safe.t -> Yojson.Safe.t option is probably very close in semantics to what you want. You have to convert your string into a string list but it exists and was added in 2.1.0, so it should be widely available.

@Shark-with-Blue-Shoes
Copy link
Author

You're right. I propose we add let member_opt k = path [k].

@Leonidas-from-XIV
Copy link
Member

My issue with it is that member_opt has wildly different semantics of the already existing member, so in an attempt to avoid confusing people I am not sure that adding it would be a good idea.

@Leonidas-from-XIV
Copy link
Member

(In any case I'll close this PR for now, the discussion on what would be the most reasonable thing to do should go into #198)

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.

2 participants