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 drop_nulls plugin and bump version #35

Merged
merged 5 commits into from Nov 20, 2023
Merged

Conversation

williamthome
Copy link
Owner

@williamthome williamthome commented Nov 20, 2023

This PR implements the drop_nulls plugin.
It ignores all keys of maps whose values are included in the 'nulls' option of the encoding.

Example

Maps

1> euneus:encode_to_binary(#{a => 1, b => undefined}, #{plugins => [drop_nulls]}).
{ok,<<"{\"a\":1}">>}

2> euneus:encode_to_binary(#{a => 1, b => undefined, c => nil}, #{nulls => [undefined, nil], plugins => [drop_nulls]}).
{ok,<<"{\"a\":1}">>}

Proplists

1> euneus:encode_to_binary([{a, 1}, {b, undefined}], #{plugins => [proplist, drop_nulls]}).
{ok,<<"{\"a\":1}">>}

2> euneus:encode_to_binary([{a, 1}, {b, undefined}, {c, nil}], #{nulls => [undefined, nil], plugins => [proplist, drop_nulls]}).
{ok,<<"{\"a\":1}">>}

Credits

Thanks both @asabil and @leonardb for the suggestions at the Erlang forums topic.

@leonardb
Copy link

leonardb commented Nov 20, 2023

Sort of surprising the the drop_nulls plugin does not drop nulls by default but converts the null into a string value "null"

4> {ok, JSON} = euneus:encode_to_binary(#{a => 1, b => undefined, c => null}, #{plugins => [drop_nulls]}).
{ok,<<"{\"a\":1,\"c\":\"null\"}">>}

edit: Should the same behavior apply to empty maps? IE, when the recursed processing of a map results in an empty map, should that be treated as a null/undefined? I'm really not sure, but it's not what I'd 'expect'

7> f(), {ok, JSON} = euneus:encode_to_binary(#{a => 1, b => undefined, c => null, e => #{f => undefined}}, #{plugins => [drop_nulls]}).          
{ok,<<"{\"a\":1,\"c\":\"null\",\"e\":{}}">>}

@williamthome
Copy link
Owner Author

@leonardb Yes, that's intentional but sounds strange due to the plugin name. The default for the nulls option is [undefined]. Maybe worth changing it to [null, undefined]? Elixir mostly uses 'nil'. Maybe then the default can be [null, undefined, nil]. What about?

@williamthome
Copy link
Owner Author

williamthome commented Nov 20, 2023

edit: Should the same behavior apply to empty maps? IE, when the recursed processing of a map results in an empty map, should that be treated as a null/undefined? I'm really not sure, but it's not what I'd 'expect'

@leonardb I'm not aware of this. I think the behavior should be the same of the Javascript:

1> JSON.stringify({})
'{}'

2> JSON.stringify({f: undefined})
'{}'

Edit

The output via this PR is:

1> euneus:encode(#{f => undefined}, #{plugins => [drop_nulls]}).
{ok,<<"{}">>}

@leonardb
Copy link

@leonardb Yes, that's intentional but sounds strange due to the plugin name. The default for the nulls option is [undefined]. Maybe worth changing it to [null, undefined]? Elixir mostly uses 'nil'. Maybe then the default can be [null, undefined, nil]. What about?

The Principle of Least Surprise (based on the name of the plugin) would seem to dictate that the default be [null, nil].

Since the initial request that launched the feature was around handling of undefined and matching the JSON.stringify behavior, maybe that should stand alone rather than part of what is really a drop_map_keys_for_matching_values plugin. Or maybe just better renaming it?

@williamthome
Copy link
Owner Author

williamthome commented Nov 20, 2023

The name is based on the nulls option, so all terms in that list would be ignored, you can define anything there and the default, currently, is [undefined].

The reason for this default is because I mostly see undefined as the "null term" for Erlang.
Javascript has null and undefined built-in, but we do not have them in Erlang.
That's the reason the nulls option exists in Euneus. I do not have another name in my mind for this option.

FWIW there is a null_term option for decoding, it also can be anything and the default is undefined, for the same reason of the nulls option.

1> {ok, JSON} = euneus:encode_to_binary(#{a => undefined, b => null, c => nil}, #{nulls => [undefined, null, nil]}).
{ok,<<"{\"c\":null,\"a\":null,\"b\":null}">>}

2> euneus:decode(JSON, #{null_term => nil}).
{ok,#{<<"a">> => nil,<<"b">> => nil,<<"c">> => nil}}

So we cannot have exactly the same behavior of the Javascript.

Edit

The null does not come from Javascript but from the null literal of JSON.

@williamthome
Copy link
Owner Author

It's a bit confusing thinking about null, undefined, nil, etc in Erlang vs JSON.

I agree with you when you say about the Principle of Least Surprise, but I'm to implement the drop_nulls plugin as it is in this PR and change it in the future if needed.

Feel free to open an issue about this. I will be happy to discuss it in a dedicated topic and with more people involved :)

@williamthome williamthome marked this pull request as ready for review November 20, 2023 17:56
@williamthome williamthome merged commit b75ab32 into main Nov 20, 2023
3 checks passed
@williamthome williamthome deleted the feat/drop-nulls branch November 20, 2023 18:36
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.

None yet

2 participants