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

New implementation of dict.update #659

Closed
wants to merge 2 commits into from

Conversation

inoas
Copy link
Contributor

@inoas inoas commented Jul 10, 2024

Replaces #610

@inoas inoas mentioned this pull request Jul 10, 2024
@inoas inoas changed the title Replace deprecated update stdlib 0.40: change the implementation of dict.update Jul 10, 2024
src/gleam/dict.gleam Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
src/gleam/dict.gleam Outdated Show resolved Hide resolved
src/gleam/dict.gleam Show resolved Hide resolved
@inoas
Copy link
Contributor Author

inoas commented Jul 10, 2024

I think this function should be called change.

I think the other implementation already done should be called update https://github.com/gleam-lang/stdlib/pull/610/files.

Update means that there already is something and it can fail to update. Update does not signal removal. Change signals removal. Change fits to this PR, update to the other.

@inoas inoas marked this pull request as draft July 10, 2024 15:39
@inoas inoas marked this pull request as ready for review July 18, 2024 18:47
Copy link
Member

@lpil lpil left a comment

Choose a reason for hiding this comment

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

Thank you

src/gleam/dict.gleam Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
src/gleam/dict.gleam Outdated Show resolved Hide resolved
src/gleam/dict.gleam Outdated Show resolved Hide resolved
src/gleam/dict.gleam Show resolved Hide resolved
@inoas
Copy link
Contributor Author

inoas commented Jul 26, 2024

@lpil are you certain about this being named update rather than change?

@lpil
Copy link
Member

lpil commented Jul 26, 2024

What's the motivation for it being change. I thought we did the whole deprecation cycle to free up the name update?

@inoas
Copy link
Contributor Author

inoas commented Jul 29, 2024

What's the motivation for it being change. I thought we did the whole deprecation cycle to free up the name update?

  1. update could be used for this impl if so desired https://github.com/gleam-lang/stdlib/pull/610/files#diff-8630634f9cc0ec3abcfa5aea55f3be0e549a2131a5cce9b7f551f880ceaaaac4R497-R502

  2. More importantly, the word update signifies that a key-pair value should exist and is updated, but the impl in this PR here may insert even if there is no key or remove if there is a key (see the test suite in this PR for examples), all depending on the change function/callback.

    It changes the dict based on the function, so IMHO change is much clearer, whereas if I read dict.update I would not assume that afterwads a key-value pair is potentially removed from the dict (or added if not existing prior).

// 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

@inoas
Copy link
Contributor Author

inoas commented Jul 29, 2024

After feedback I made the changes to return Option rather than Result and tried to simplify the docs and the unit tests

@inoas
Copy link
Contributor Author

inoas commented Sep 26, 2024

This is still ready to be reviewed/merged unless you want me to rename this to change and have update do the other impl (see above). Please let me know if there are still things to do.

@inoas inoas requested a review from lpil September 26, 2024 15:45
@inoas
Copy link
Contributor Author

inoas commented Oct 31, 2024

Any chance to get this into 0.42 with either name change or name update, it has been ready for a few month now @lpil

@inoas
Copy link
Contributor Author

inoas commented Nov 18, 2024

I have removed conflicts with recent changes on main branch

@lpil any news on this PR?

I am hoping for one of these:

  • merge as is
  • rename from update to change (my vote)
  • discard PR

@inoas inoas changed the title stdlib 0.40: change the implementation of dict.update New implementation of dict.update Dec 9, 2024
@inoas inoas force-pushed the replace-deprecated-update branch from f3d7e38 to 4bab4b7 Compare December 9, 2024 13:51
@inoas inoas force-pushed the replace-deprecated-update branch from 4bab4b7 to 1959ea6 Compare December 9, 2024 13:53
@inoas
Copy link
Contributor Author

inoas commented Dec 9, 2024

I have resolved the merge conflicts another time.

Is this desired (as update() or as change())?

@lpil
Copy link
Member

lpil commented Dec 17, 2024

Hello! Thank you for this but I've not yet had any reports of real applications feeling they need this yet so I would like to wait until that demand is clear.

@inoas
Copy link
Contributor Author

inoas commented Dec 19, 2024

Hello! Thank you for this but I've not yet had any reports of real applications feeling they need this yet so I would like to wait until that demand is clear.

OK, someone in future can implement it as change() (or update()) if demand is noticed.

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