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

Map.delete and pure/Map.delete have inconsistent semantics #160

Open
crusso opened this issue Feb 14, 2025 · 4 comments
Open

Map.delete and pure/Map.delete have inconsistent semantics #160

crusso opened this issue Feb 14, 2025 · 4 comments

Comments

@crusso
Copy link
Contributor

crusso commented Feb 14, 2025

The former traps deleting an absent key, the latter does not.

I suggest

add/remove trapping
put/delete non-trapping

@rvanasa
Copy link
Collaborator

rvanasa commented Feb 20, 2025

What about add() : Bool and remove() : Bool (non-trapping)? This would make is so that the developer must either explicitly ignore the expression or provide some sort of error handling logic. We could also include addChecked() : () and removeChecked() : () (trapping), although I think it would be better to discourage trapping in favor of giving the developer a way to handle the error in context.

Another variation is add() : () and remove() : () (non-trapping) with addSafe() : Bool and removeSafe() : Bool (also non-trapping), along with the trapping functions if we want to include those.

The motivation is for addAll(), removeAll(), and retainAll() to correspond in a more intuitive way to union(), diff(), and intersect().

It might be good to keep "add" and "remove" (or I suppose "put" and "delete") in both of the names if possible, since otherwise it could be confusing to new developers who might mix up which functions are paired with each other.

@crusso
Copy link
Contributor Author

crusso commented Feb 20, 2025

Boolean add and remove would solve the problem but might lead to awkward code with all those ignores in the common case. And would 'addAll' etc return a bool and if so, with what semantics ?

Maybe just non-trapping add/remove
with boolean put/delete?

I was also wondering if we could use

Union/intersection/difference (creating a new map)
And
Unite/intersect/diff for inplace update (in the imperative API)

Let's talk tomorrow...

@rvanasa
Copy link
Collaborator

rvanasa commented Feb 20, 2025

Maybe just non-trapping add/remove with boolean put/delete?

I'd be ok with this, although a remaining concern is that it might be confusing to remember which ones return a boolean.

Unite/intersect/diff for inplace update (in the imperative API)

In this case, we might want to rename the functions sort() -> sorted() and sortInPlace() -> sort(). Otherwise, I suppose unionInPlace(), intersectInPlace(), and diffInPlace() (or with the words fully spelled out) could also work as an admittedly verbose naming convention.

Overall, I've been choosing names based on consistency first, clarity second, and conciseness third as an explanation for thinking *InPlace() could make sense despite being verbose. This also comes from the Python ecosystem where data structures (primarily dataframes) have in-place operations such as df.sort(..., inplace=True).

@crusso
Copy link
Contributor Author

crusso commented Feb 20, 2025

The other thing that strikes me is that the Boolean ones are truly awkward in the functional api since you need to return a new map and a Boolean. Wouldn't be so bad if we had subtyping on tuples (you could ignore the second value) but we don't..
Let's do something else in the meantime and discuss tomorrow

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

No branches or pull requests

2 participants