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

Rename unshift!/shift! to pushfirst!/popfirst! #25100

Merged
merged 1 commit into from
Dec 21, 2017

Conversation

c42f
Copy link
Member

@c42f c42f commented Dec 15, 2017

Well, perhaps this won't fly, but emboldened by @timholy and @KristofferC's support among other people, here's a possible fix for #23902

Some points of support for doing this:

  • This is a relatively minimal change. In particular it doesn't affect push! and pop! which are widely used and agreed upon terms which apply to many data structures where pushfront/popfront! don't make sense.
  • These names have enough symmetry with push! and pop!, that you can tell whether they're adding or removing elements if you know what push and pop themselves do. This is an improvement on the current state of affairs, where there's really nothing in the name which tells you that unshift! adds elements. You just have to know, in addition to knowing about push!.
  • There's some precedent, with C++ using the names push_front and pop_front Not quite, if we're going with pushfirst! and popfirst!

Perhaps surprisingly, while making some of the changes here manually I kept having to remind myself that unshift (rather than shift) adds elements. So I started again and did most of it with sed as a safer option.

@andyferris andyferris added collections Data structures holding multiple items, e.g. sets deprecation This change introduces or involves a deprecation triage This should be discussed on a triage call labels Dec 15, 2017
@andyferris
Copy link
Member

FWIW I support this change for the reasons mentioned. To me, the less new words a user needs to learn to use the language, the better, and shift/unshift in particular seem to me at least to be historical baggage rather than adding insightfulness.

@Sacha0
Copy link
Member

Sacha0 commented Dec 15, 2017

Though I prefer pushfront!/popfront! to unshift!/shift!, I might advocate a slight additional tweak. @StefanKarpinski's objection to front/back terminology

I'm not sure that front and back are clearer than left and right. Of course, for left/right terminology it's a bit unfortunate that we print vectors vertically, but front/back aren't better in that respect.

stands. A hopefully broadly amicable tweak that preserves the feel of pushfront!/popfront! while addressing Stefan's objection is pushfirst!/popfirst!, which has the strong advantage over pushfront!/popfront! of fitting beautifully with existing terminology and definitions in Julia and thereby eschewing ambiguity:

julia> foo = [1, 2, 3]
3-element Array{Int64,1}:
 1
 2
 3

julia> first(foo)
1

julia> last(foo)
3

such that with pushfirst!/popfirst!:

julia> pushfirst!(foo, 0)
4-element Array{Int64,1}:
 0
 1
 2
 3

julia> first(foo)
0

julia> popfirst!(foo)
0

and likewise for the natural symmetric partners pushlast!/poplast!, which push!/pop! could alias

julia> pushlast!(foo, 4) # or push!(foo, 4)
4-element Array{Int64,1}:
 1
 2
 3
 4

julia> last(foo)
4

julia> poplast!(foo) # or pop!(foo)
4

Best! :)

@StefanKarpinski
Copy link
Sponsor Member

I very much do not care for pushfront! and popfront! but pushfirst! and popfirst! I could get behind. They could potentially be abbreviated to pushf! and popf!, satisfying both camps, but perhaps that's too cryptic (and the corresponding pushl! and popl! where the l stands for last would be unfortunate since it could also mean left).

@vtjnash
Copy link
Sponsor Member

vtjnash commented Dec 15, 2017

I'm not sure that front and back are clearer than left and right. Of course, for left/right terminology it's a bit unfortunate that we print vectors vertically, but front/back aren't better in that respect.

Also, in a world where we finally implement pushleft / pushright / pushaxes for Matrixes (also known as version 1.1), this might be an unfortunate in hindsight (in the future).

@omus
Copy link
Member

omus commented Dec 15, 2017

I also don't care for pushfront! and popfront!. If we're going in the direction of pushfirst! and popfirst! I would rather go with push!(first, ...) and pop!(first, ...). I added my suggestion to the original issue.

@Sacha0
Copy link
Member

Sacha0 commented Dec 15, 2017

From the preceding responses, pushfirst!/popfirst! seems like a more broadly amicable compromise so far? :)

@c42f c42f changed the title Rename unshift!/shift! to pushfront!/popfront! Rename unshift!/shift! to pushfirst!/popfirst! Dec 16, 2017
@c42f
Copy link
Member Author

c42f commented Dec 16, 2017

Excellent, I think we have something worthwhile in pushfirst! and popfirst! - I've updated the PR to that suggestion, thanks @Sacha0.

@c42f
Copy link
Member Author

c42f commented Dec 16, 2017

Also thanks @vtjnash for that hint of prehindsight :-)

@Sacha0
Copy link
Member

Sacha0 commented Dec 16, 2017

Seems to be in shape apart from the minor conflict in base/deprecated? (Inserting your deprecations at a random location in base/deprecated's body rather than at its ends helps avoid rapidly accruing such conflicts.) Thanks @c42f! :)

@omus
Copy link
Member

omus commented Dec 16, 2017

Just a quick follow up: why is pushfirst!(...) preferred over push!(first, ...)?

@rfourquet
Copy link
Member

Just a quick follow up: why is pushfirst!(...) preferred over push!(first, ...)?

why do you prefer push!(first, ...) over pushfirst!(...) ? 😄

@omus
Copy link
Member

omus commented Dec 16, 2017

why do you prefer push!(first, ...) over pushfirst!(...) ? 😄

I prefer using first as a parameter as this would mean the documentation for push! would cover both pushing to the first or last elements.

Also, if people discover pushfirst! before push! they probably will expect to find pushlast!. If we go with push!(first, ...) we could have push!(last, ...) as an alias for push!(...).

@Sacha0
Copy link
Member

Sacha0 commented Dec 16, 2017

I prefer using first as a parameter as this would mean the documentation for push! would cover both pushing to the first or last elements.

The docstrings for push!/pop! can link to pushfirst!/popfirst! and the hypothetical pushlast!/poplast!?

Also, if people discover pushfirst! before push! they probably will expect to find pushlast!. If we go with push!(first, ...) we could have push!(last, ...) as an alias for push!(...).

pushlast!/poplast! can just as well be aliases for push!/pop! or vice versa?

To be clear, I lack strong sentiments on {push|pop}!({first|last}, ... versus {push|pop}{first|last}!(.... I see little if any meaningful difference between those two spellings (though the former breaks the mutated-argument-first convention, doesn't behave like most other functions accepting a function argument, and might be something of a pun). The latter seems broadly amicable, hence I advocate seizing it and moving on to more important issues :). Best!

@omus
Copy link
Member

omus commented Dec 16, 2017

breaks the mutated-argument-first convention, doesn't behave like most other functions accepting a function argument, and might be something of a pun

Ah, I didn't think of this. Thanks for the reply @Sacha0.

@StefanKarpinski
Copy link
Sponsor Member

The reason I don't like push!(first, ...) is that it's not really a higher order function – it's just using the fact that we can dispatch on function types to make a syntax pun. It's not like you can pass any other function besides first and last in there.

@c42f
Copy link
Member Author

c42f commented Dec 16, 2017

CI roulette has passed.

I'll rebase now to fix the conflict.

@StefanKarpinski
Copy link
Sponsor Member

StefanKarpinski commented Dec 20, 2017

While being tired and looking at some tests using unshift! and having to wonder for a while whether they push or pop values from the front of the array in question, I'm now fully in favor of this rename – I shouldn't need that much mental power to remember what these functions do. I vote that we go ahead with this.

@timholy
Copy link
Sponsor Member

timholy commented Dec 20, 2017

There are a couple of new (un)shift! uses in test/compile.jl to pick up (sorry).

@c42f
Copy link
Member Author

c42f commented Dec 20, 2017

Thanks Tim, I'll rebase and clean those up as well.

@StefanKarpinski StefanKarpinski added this to the 1.0 milestone Dec 21, 2017
@StefanKarpinski StefanKarpinski removed the triage This should be discussed on a triage call label Dec 21, 2017
@JeffBezanson
Copy link
Sponsor Member

Triage says 👍

@StefanKarpinski StefanKarpinski merged commit 1e0d26b into JuliaLang:master Dec 21, 2017
@c42f c42f deleted the deprecate-unshift branch January 2, 2018 11:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
collections Data structures holding multiple items, e.g. sets deprecation This change introduces or involves a deprecation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants