-
Notifications
You must be signed in to change notification settings - Fork 10
Implement with/2 and without/2 #36
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
Conversation
|
@mrallen1 just a quick WIP implementation that I think does all the things for with, based on checking: 6> Object = #{<<"foo">> => #{<<"bar">> => 1, <<"baz">> => 1, <<"fum">> => [1,2,3]}}.
#{<<"foo">> =>
#{<<"bar">> => 1,<<"baz">> => 1,<<"fum">> => [1,2,3]}}
7> jsn:with([<<"foo.bar">>, <<"foo.baz">>, <<"foo.fum.nope">>, {<<"foo">>, <<"fum">>, last},{<<"foo">>, <<"fum">>, first}], jsn:as_proplist(Object)).
[{<<"foo">>,
[{<<"bar">>,1},{<<"baz">>,1},{<<"fum">>,[1,3]}]}]
8> jsn:with([<<"foo.bar">>, <<"foo.baz">>, <<"foo.fum.nope">>, {<<"foo">>, <<"fum">>, 2},{<<"foo">>, <<"fum">>, first}], jsn:as_proplist(Object)).
[{<<"foo">>,
[{<<"bar">>,1},{<<"baz">>,1},{<<"fum">>,[1,2]}]}]
9> jsn:with([<<"foo.bar">>, <<"foo.baz">>, <<"foo.fum.nope">>, {<<"foo">>, <<"fum">>, 2},{<<"foo">>, <<"fum">>, first}], Object).
#{<<"foo">> =>
#{<<"bar">> => 1,<<"baz">> => 1,<<"fum">> => [1,2]}}Before I write unit tests, I am curious if you agree with this approach. Note that it takes any JSON value, and if you ask for paths in something that doesn't have paths, you get that thing back. So like, if an intermediate element of your path is an array (or value), you just get that thing back at that layer, and it doesn't dig further or raise an exception. I'm not sure what the right approach is. Raising exceptions sounds annoying as heck, but also it sounds tedious to take a set of deep paths and give back a shallow object with only top-level matches without telling the user, "OK, I did what you asked, but you might not like it—maybe you should validate your inputs better!" |
|
... Obviously, code organization, documentation, unit tests are all TODO. |
|
I think the behavior of |
va17
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First take LGTM. I also think we should go down the path of mimicking maps:with/2 and not return errors, just ignore missing paths.
4243e38 to
b9687a6
Compare
|
OK, sounds like my implementation is on the right track. I added |
12d5a0a to
99b4088
Compare
|
OK, implementation and unit tests done, along with basic function documentation. Will add a README section for the 2 functions as well, along with the fun caveats about optimization (pass a pre-computed path elements map when applying repeatedly) and atom keys (only supports atom keys that are in the atom table when the path elements map is constructed, and won't work if applied to structures with atom keys that are added to the atom table later (TL;DR: don't use atom keys in JSON representation). Curious what you think of the implementation. |
|
Alright—I've written the README. I think this is ready to be reviewed as soon as you have the time, @mrallen1. |
9dba616 to
88bf324
Compare
jadeallenx
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great. ![]()
rmpalomino
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some pedantic comments.
| structure. That means that if you call this function before constructing your | ||
| object, and you use new atoms when doing so, those atom-keyed path elements | ||
| may not be included (or excluded) as expected. If your objects have atoms, | ||
| it is recommended to ensure all atoms are referenced in the system (i.e., in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it actually feasible to check if an atom is in the atom table? I feel like like the recommendation here is better left out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, binary_to_existing_atom/2 is how we check. As for ensuring that they're present, you just have to reference them in compiled code. This is not better left out IMO.
Resolves #35. See README updates in the commit for implementation notes.
Resolves #35.
See README updates in the commit for implementation notes.