base64: Allow options list for encode/decode functions

[juhlig]

The encode/decode functions currently accept only an option map. While this is fine, it is different from how most other functions take options, which is an option list. This PR adds the ability to provide options as a list or a map. Also fixed some typos in the docs while I was at it.

[github-actions]

CT Test Results

    2 files     97 suites   1h 7m 0s ⏱️
2 226 tests 2 174 ✅ 51 💤 1 ❌
2 617 runs  2 560 ✅ 56 💤 1 ❌

For more details on these failures, see this check.

Results for commit 1995a47.

♻️ This comment has been updated with latest results.

To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass.

See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally.

Artifacts

• Complete CT logs (Download Logs)
• HTML Documentation (Download HTML Docs)
• No Windows Installer found

// Erlang/OTP Github Action Bot

[MarkoMin]

I wonder whats the need behind this? Is this just optimization for slightly smaller memory footprint or something else?

[juhlig]

I wonder whats the need behind this? Is this just optimization for slightly smaller memory footprint or something else?

Something else: API consistency. Basically every other function in every other module that takes options takes them as a list (and only sparsely additionally as a map), even fairly new ones like sets:new/1 or sets:from_list/2. The encode/decode functions in base64 are standing out in that they take options exclusively as a map.

[Maria-12648430]

Something else: API consistency.

I support that 👍

[nickva]

It seems newer modules usually end up with option maps like the socket one socket:/open/2, if anything, sets:new/1 seems out of place.

I think it would seem a bit odd to have two ways to specify options, that in itself is an inconsistency...unless one way is officially deprecated (maybe it should be?). Or maybe this dual option format should be extended to other API (socket, etc).

[Maria-12648430]

It seems newer modules usually end up with option maps like the socket one socket:/open/2, if anything, sets:new/1 seems out of place.

I think it would seem a bit odd to have two ways to specify options, that in itself is an inconsistency...unless one way is officially deprecated (maybe it should be?). Or maybe this dual option format should be extended to other API (socket, etc).

Well, I see your point, but I would argue that it is not really odd to have two ways to say the same thing but that it is odd to have to use this way here and another way there, depending on the module (or whether the module is older or newer).

I would go with the suggestion to use the "dual way" wherever possible.

Which basically means that option lists should be possible everywhere (on top of what maps offer, they "support" option ordering and multiple same-named options, something that you need for example in gen_tcp for raw options), but not necessarily maps (for that reason).

Multiple same-named options are of course not always a good thing if they are not meant to be. It opens up another avenue for inconsistencies, some (most?) modules use the first, some (gen_tcp comes tonmind) use the last...

[juhlig]

I would go with the suggestion to use the "dual way" wherever possible.

I support that 🤗

[MarkoMin]

Consistency is something I'd always go for, but in the first place we'd need to agree on what pattern should be used in the whole codebase.

It doesn't bother me to make it work "dual way", but implementation might be tricky and if that becomes a pattern it might cause maintenance issues.

See for example this piece of code:

get_decoding_offset(#{mode := standard}) -> 1;
get_decoding_offset(#{mode := urlsafe}) -> 257;
get_decoding_offset(#{}) -> 1.
get_decoding_offset(#{}) -> 1;
get_decoding_offset(Opts) when is_list(Opts) ->
    case lists:keyfind(mode, Opts) of
	{mode, standard} -> 1;
	{mode, urlsafe} -> 257;
	_ -> 1
    end.

What if we have 10 modes - you have do insert your logic in 2 clauses per mode which is not something you'd want. This is one option, imagine this for N options. Now this is only getter funcion, now imagine validation functions which are pretty common to. In the whole code, you'll end up supporting both versions of options...

Even if you want your API to be consistent, can't we just stick to one internal representation and use maps:from/to_list/1?

[max-au]

I recall there used to be a guideline document from OTP team (or was it me writing it? can't remember now). It was about the choice of "map" vs "proplist" for any new API. If memory serves me well:

• in all the new APIs, proplist should be used only when the order of supplied options is important
• in all other cases, use map

For and existing function, it's a case-by-case decision. It may be more useful to introduce a new clause/overload that accepts a map with all existing options.

In my opinion, these guidelines make sense. With proplists it's easy to run into a conflicting behaviour. Example:
module:function([{option, true}, {option, false}]) - should option be true or false?

[starbelly]

I recall there used to be a guideline document from OTP team (or was it me writing it? can't remember now). It was about the choice of "map" vs "proplist" for any new API. If memory serves me well:

• in all the new APIs, proplist should be used only when the order of supplied options is important

• in all other cases, use map

✅ https://erlangforums.com/t/handling-options-in-erlang-otp-apis/1133

[MarkoMin]

✅ https://erlangforums.com/t/handling-options-in-erlang-otp-apis/1133

Maybe this should be summarized/mentioned in CONTRIBUTING.md ? Seems like this question occurs from time to time and having it directly in the OTP directory would be beneficial for future contributors.

[juhlig]

Consistency is something I'd always go for, but in the first place we'd need to agree on what pattern should be used in the whole codebase.

Ok, agreed. Guess I picked the wrong end of the stick by trying to make new APIs work in old ways XD

It doesn't bother me to make it work "dual way", but implementation might be tricky and if that becomes a pattern it might cause maintenance issues.

It may make most sense to make new APIs map-only (if possible, ie if option order doesn't matter) and add option maps additionally to old APIs that currently only support lists. This is likely a tedious task (which I guess @Maria-12648430 would enjoy nevertheless), but it could be done on a per-module basis, or if the need arises. How exactly such maps can/will have to look like is module- or even function-specific.

A tricky point might be figuring out which modules to start out with. Some modules take options to pass them on in call to other functions in other modules (eg gen_*:start_* -> gen:start -> proc_lib:start_* -> erlang:spawn_opt), so can't be changed to accept maps before the called functions in other modules have been changed to accept maps.

What if we have 10 modes - you have do insert your logic in 2 clauses per mode which is not something you'd want. This is one option, imagine this for N options. Now this is only getter funcion, now imagine validation functions which are pretty common to. In the whole code, you'll end up supporting both versions of options...

This can of course be done in a multitude of ways, this was not meant to be a general pattern.

Even if you want your API to be consistent, can't we just stick to one internal representation and use maps:from/to_list/1?

Not generally, no. For example, most (not all) option lists are proplists-like in that the first of repeated options "wins". Using maps:from_list/1 however, the last ends up in the map. There is a function proplists:to_map/1,2 that works in this case. Then again, the options list may not really be a proplist, eg in cases it may be allowed to contain >2-tuples. Etc.

Anyway. I guess I'll close this PR in a few days and open a new one with only the typo corrections in it. Thanks for your opinions everyone.

[MarkoMin]

P.S.: #10502