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

take -> pop #12678

Merged
merged 1 commit into from
Nov 20, 2019
Merged

take -> pop #12678

merged 1 commit into from
Nov 20, 2019

Conversation

c-blake
Copy link
Contributor

@c-blake c-blake commented Nov 17, 2019

Discussion both in
#12600
and in
https://forum.nim-lang.org/t/5499
indicates that everyone is happy/happier with pop.

This just renames the brand new takes to pop and installs inline
aliases/wrappers to preserve Table.take and TableRef.take.

Update apis.rst to try to maintain consistency of remove-and-return procs.

    #12600
and in
    https://forum.nim-lang.org/t/5499
indicates that everyone is happy/happier with ``pop``.

This just renames the brand new ``take``s to ``pop`` and installs inline
aliases/wrappers to preserve ``Table.take`` and ``TableRef.take``.

Update apis.rst to try to maintain consistency of remove-and-return procs.
@c-blake c-blake changed the title Discussion both in take -> pop Nov 17, 2019
@c-blake
Copy link
Contributor Author

c-blake commented Nov 17, 2019

NOTE: for the various tables it might also be nice to have an "unkeyed pop" that like HashSet.pop just removes & returns the first iteration-order (key, value) pair. This PR is simpler than that and leaves room for such an unkeyed pop overload to be added later.

result = t[].take(key, val)
result = t[].pop(key, val)

proc take*[A, B](t: TableRef[A, B], key: A, val: var B): bool {.inline.} =
Copy link
Contributor

Choose a reason for hiding this comment

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

I would deprecate this while you're at it.

Copy link
Contributor Author

@c-blake c-blake Nov 17, 2019

Choose a reason for hiding this comment

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

I don't disagree as indicated by prior PR thread, but @Araq reacted negatively to that idea in the linked forum thread. So, I didn't.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It may simply be the price we pay for not fixing Table.take for 2 years with a 1.0 transition in there to maintain it forever as an alias. I did demote it in the docs, though. After some purgatory period in that alias-in-docs state, it could maybe become deprecated for another period before final removal. That might or might not be a good model for how to revise stdlib things in the 1.0 era.

@planetis-m
Copy link
Contributor

This is confusing, as seq.pop returns the element while, take returns a bool. In my opinion there are different operations and need to have different names.

@c-blake
Copy link
Contributor Author

c-blake commented Nov 17, 2019

Well, Nim has all kinds of type-overloaded APIs and users of APIs do need to be aware of the parameter types. Unlike seq.pop, in this case the key may have no associated value in this initial case. However, as I mentioned above, this same name may be associated with an "unkeyed" overload pop(Table): tuple[K,V] that removes & returns the first iteration-order entry. I think "remove & return" is common enough to stick with one name for that abstract operation and many others seem to agree in various discussions. You are, to my knowledge, the lone dissenting voice unless someone changes their mind.

@c-blake
Copy link
Contributor Author

c-blake commented Nov 17, 2019

The above said, there has been a lot of back & forth about bool/status-code-oriented or Option[T]-oriented or exception-oriented interfaces which may relate to your objection, but @narimiran was just following the lone Table.take before. So, pop(Table[K,V], K): V with an exception wouldn't be crazy and pop(Table) alone might raise an exception on an empty table. Option[T] might be more dependency than is desired, though.

@planetis-m
Copy link
Contributor

val is an out parameter, I missed that, then ok.

@c-blake
Copy link
Contributor Author

c-blake commented Nov 19, 2019

@Araq - anything else you need/want?

@Araq Araq merged commit a880041 into nim-lang:devel Nov 20, 2019
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.

4 participants