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

Force push! and unshift! to need at least one item #10400

Merged
merged 1 commit into from
Mar 5, 2015
Merged

Conversation

jakebolewski
Copy link
Member

Is there any reason for this fallback (I'm guessing to support splatting of potentially zero arguments)? Otherwise it can be a source of subtle bugs.

cc @andreasnoack

@tkelman
Copy link
Contributor

tkelman commented Mar 4, 2015

if you're going to push! a splatted collection onto something, you should be using append! anyway

Should there be a specific error message suggesting to do so?

@StefanKarpinski
Copy link
Member

I.e. should we add a deprecation for this? It's hard to say since if it gets triggered, we may suspect that someone is splatting values into a push! call, but we can't be sure.

@mbauman
Copy link
Member

mbauman commented Mar 4, 2015

It seems like it'd be a good Lint message. @tonyhffong

julia> lintstr("x=[1,2,3]; push!([], x...)")
0-element Array{Lint.LintMessage,1}

@ivarne
Copy link
Member

ivarne commented Mar 4, 2015

+1 for the API cleanup.

Depreciation seems to be in order. We're definitely not in a hurry to potentially break peoples code for this.

@IainNZ
Copy link
Member

IainNZ commented Mar 4, 2015

I like this, because I've accidentally done this (1 arg push) and didn't get a method error - I'd have liked one.

@JeffBezanson
Copy link
Member

Gotta love "0, 1, or N" problems :)
+1 for a deprecation

@andreasnoack
Copy link
Member

@IainNZ That was exactly the motivation. Last night and this morning were spent figuring out why my code was numerically unstable only to realize that I didn't push anything to the vector that should keep track of the error growth.

ivarne added a commit that referenced this pull request Mar 5, 2015
Force push! and unshift! to need at least one item
@ivarne ivarne merged commit 27b6bbf into master Mar 5, 2015
@ivarne ivarne deleted the jcb/pushchg branch March 5, 2015 05:30
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.

8 participants