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

fix #12519: introduce OrderedTable.take, CountTable.del, CountTable.take #12600

Merged
merged 4 commits into from
Nov 8, 2019

Conversation

narimiran
Copy link
Member

@narimiran narimiran commented Nov 5, 2019

Rationale: these procs exist for Table, they should also exist for OrderedTable and CountTable.

Fixes #12519.

@Araq
Copy link
Member

Araq commented Nov 6, 2019

Needs the upcoming .since annotation. So it's delayed for now.

@narimiran
Copy link
Member Author

Needs the upcoming .since annotation. So it's delayed for now.

Done.

@Araq
Copy link
Member

Araq commented Nov 8, 2019

No changelog entry?

@narimiran
Copy link
Member Author

No changelog entry?

Done :)

@Araq Araq merged commit 6958248 into nim-lang:devel Nov 8, 2019
@c-blake
Copy link
Contributor

c-blake commented Nov 9, 2019

I realize this naming is patterned after the 2 year old Table.take, but Nim's seq and HashSet also already have similar remove-and-return operations called pop. I didn't say anything about Table.take back then, but thought it worth bringing up before we solidify this naming in the Table family.

Also, this operation is not really covered in doc/apis.rst, but should be. I also think the naming analogy with seq makes it more intuitive that pop might need to be called multiple times to undo a series of same-key adds. Tagging @cdunn2001 since he was recently updating take documentation along those lines.

(Less relevant than Nim-internal consistency, but Python also calls it pop, and take seems more a verb from functional programming language communities which Nim does seem to imitate more generally.)

My vote would be to name this pop and have Table.take either become deprecated or just be along-for-the-ride-forever for the sake of 1.0 series backward compatibility.

@c-blake
Copy link
Contributor

c-blake commented Nov 9, 2019

(And of course there are also pop variations in heapqueue.nim and deques.nim. It sure feels like pop is the super majority vote internally while take was a singleton until this PR.)

@c-blake c-blake mentioned this pull request Nov 17, 2019
Araq pushed a commit that referenced this pull request Nov 20, 2019
#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.
@narimiran narimiran deleted the tables-fix branch November 26, 2019 14:34
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.

tables.take() is defined only for Table and missed for other table containers
3 participants