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

stdlib 0.40: change the implementation of dict.update #659

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
- The `function.curry*` and `function.apply*` functions have been deprecated.
- The deprecated `dynamic.unsafe_coerce` function has been removed.
- The deprecated `dict.update` function has been removed.
- The dict module gains a new `update` function.
- The deprecated `order.max` and `order.min` functions have been removed.
- The `float` module gains the `modulo` function.
- The `uri.origin` function no longer incorrectly has a trailing slash.
Expand Down
63 changes: 61 additions & 2 deletions src/gleam/dict.gleam
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import gleam/option.{type Option}
import gleam/option.{type Option, None, Some}

/// A dictionary of keys and values.
///
Expand Down Expand Up @@ -60,7 +60,7 @@ pub fn is_empty(dict: Dict(k, v)) -> Bool {
///
/// ## Examples
///
/// Calling `to_list` on an empty `dict` returns an empty list.
/// Calling `to_list` on an empty dict returns an empty list.
///
/// ```gleam
/// new() |> to_list
Expand Down Expand Up @@ -474,6 +474,65 @@ pub fn upsert(
|> insert(dict, key, _)
}

/// Changes the value in a dict for a given key using a given function.
///
/// Returns a dict where the caller attempts to change a key-value pair in the
/// dict bei either setting or updating or removing or ignoring a key-value
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not happy with this explanation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any better ideas?

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps something more concise?

Updates the value in a dict for a given key using a given function.

If there already was a value in the dict for the key then it is passed into the function.

If a value is returned from the function then it will be used as the new value in the dict. If no value is returned then any previous value is removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If a value is returned from the function then it will be used as the new
value in the dict. If no value is returned then any previous value is removed.

But is this true?

The change callback can on None case add the key by returning Ok(value) (or Some(value))? Or did I miss something.

I am updating the tests so it is clearer that the change function is very powerful and flexible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This shows an example where I cannot see that the function description above matches. Because it really depends on the change callback/function what happens here.

https://github.com/gleam-lang/stdlib/pull/659/files#diff-99caf068f3c916909bcf3231d839db8a8afdebb65dc04d0353eea53071729264R265-R278

/// pair; in case:
///
/// 1. the `update` key is present in the dict, then the value passed to
/// function `with` is `Some(value)`; then:
/// 1. if `fun` returns `Ok(value)`, then the `update` key is newly
/// associated with to the value of `value` of `Ok(value)`.
/// 2. else the `update` key and its associated value are removed from the
/// dict.
/// 2. the `update` key is not present in the dict, then the `value` passed to
/// function `with` is `None`; then:
/// 1. if `fun` returns `Ok(value)`, then the `update key` is added to the
/// dict associated with the `value` of `Ok(value)`.
/// 2. else the `update key` is not added to the map.
///
/// ## Example
///
/// ```gleam
/// let dict = dict.from_list([#("a", 0), #("b", 1), #("c", 2)])
///
/// let inc_if_exists_or_discard = fn(x) {
/// case x {
inoas marked this conversation as resolved.
Show resolved Hide resolved
/// Some(i) -> Ok(i + 1)
/// None -> Error(Nil)
/// }
/// }
///
/// dict
/// |> dict.update("a", inc_if_exists_or_discard)
/// // -> from_list([#("a", 1), #("b", 1), #("c", 2)])
///
/// dict
/// |> dict.update("e", inc_if_exists_or_discard)
/// // -> from_list([#("a", 0), #("b", 1), #("c", 2)])
/// ```
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this shows the functionality very clearly. One example per code block would be good

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not certain I follow. Per which code block?

///
pub fn update(
in dict: Dict(k, v),
update key: k,
with fun: fn(Option(v)) -> Result(v, Nil),
) -> Dict(k, v) {
Copy link
Member

Choose a reason for hiding this comment

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

Could you use Option for the return value here please

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought we are not using Option for Return values.
If we have regular functions following that pattern, we then cannot apply those in this update function?

case do_get(dict, key) {
Ok(existing_value) ->
case fun(Some(existing_value)) {
Ok(value) -> do_insert(key, value, dict)
Error(_) -> do_delete(key, dict)
}
Error(_) -> {
case fun(None) {
Ok(value) -> do_insert(key, value, dict)
Error(_) -> dict
}
}
}
}

fn do_fold(list: List(#(k, v)), initial: acc, fun: fn(acc, k, v) -> acc) -> acc {
case list {
[] -> initial
Expand Down
99 changes: 99 additions & 0 deletions test/gleam/dict_test.gleam
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,105 @@ pub fn upsert_test() {
|> should.equal(dict.from_list([#("a", 0), #("b", 1), #("c", 2), #("z", 0)]))
}

pub fn update_test() {
let dict = dict.from_list([#("a", 0), #("b", 1), #("c", 2)])

let inc_if_exists_or_set = fn(x) {
case x {
Some(i) -> Ok(i + 1)
None -> Ok(1)
}
}

dict
|> dict.update("a", inc_if_exists_or_set)
|> should.equal(dict.from_list([#("a", 1), #("b", 1), #("c", 2)]))

dict
|> dict.update("e", inc_if_exists_or_set)
|> should.equal(dict.from_list([#("a", 0), #("b", 1), #("c", 2), #("e", 1)]))

let discard_if_exists_else_noop = fn(x) {
case x {
Some(_i) -> Error(Nil)
None -> Error(Nil)
}
}

dict
|> dict.update("a", discard_if_exists_else_noop)
|> should.equal(dict.from_list([#("b", 1), #("c", 2)]))

dict
|> dict.update("e", discard_if_exists_else_noop)
|> should.equal(dict.from_list([#("a", 0), #("b", 1), #("c", 2)]))

let inc_if_exists_else_noop = fn(x) {
case x {
Some(i) -> Ok(i + 1)
None -> Error(Nil)
}
}

dict
|> dict.update("a", inc_if_exists_else_noop)
|> should.equal(dict.from_list([#("a", 1), #("b", 1), #("c", 2)]))

dict
|> dict.update("e", inc_if_exists_else_noop)
|> should.equal(dict.from_list([#("a", 0), #("b", 1), #("c", 2)]))

let discard_if_exists_else_set_to_zero = fn(x) {
case x {
Some(_i) -> Error(Nil)
None -> Ok(0)
}
}

dict
|> dict.update("a", discard_if_exists_else_set_to_zero)
|> should.equal(dict.from_list([#("b", 1), #("c", 2)]))

dict
|> dict.update("e", discard_if_exists_else_set_to_zero)
|> should.equal(dict.from_list([#("a", 0), #("b", 1), #("c", 2), #("e", 0)]))

let discard_if_exists_else_set_to_zero = fn(x) {
case x {
Some(_i) -> Error(Nil)
None -> Ok(0)
}
}

dict
|> dict.update("a", discard_if_exists_else_set_to_zero)
|> should.equal(dict.from_list([#("b", 1), #("c", 2)]))

dict
|> dict.update("e", discard_if_exists_else_set_to_zero)
|> should.equal(dict.from_list([#("a", 0), #("b", 1), #("c", 2), #("e", 0)]))

// Utilize a function returning a `Result` in the change function.
let half_or_zero_else_noop = fn(x) {
case x {
Some(i) -> int.divide(i, 2)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because other functions usually return Result rather than Option, I have opted for the change function to return Result not Option as well.

I have added a unit test example to show what I mean.

Are you certain you want to return Option instead?

ref #659 (comment) cc @lpil

None -> Error(Nil)
}
}

dict
|> dict.update("c", half_or_zero_else_noop)
|> should.equal(dict.from_list([#("a", 0), #("b", 1), #("c", 1)]))

dict
|> dict.update("b", half_or_zero_else_noop)
|> should.equal(dict.from_list([#("a", 0), #("b", 0), #("c", 2)]))

dict
|> dict.update("e", half_or_zero_else_noop)
|> should.equal(dict.from_list([#("a", 0), #("b", 1), #("c", 2)]))
}

pub fn fold_test() {
let dict = dict.from_list([#("a", 0), #("b", 1), #("c", 2), #("d", 3)])

Expand Down
Loading