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

delete replace!(pred, A, new) #27944

Merged
merged 1 commit into from
Jul 29, 2018
Merged

delete replace!(pred, A, new) #27944

merged 1 commit into from
Jul 29, 2018

Conversation

rfourquet
Copy link
Member

@rfourquet rfourquet commented Jul 5, 2018

replace!(pred, A, new) replaces all values in A for which pred is true by the value new. It's a very thin wrapper over replace!(new, A).

It was noted in #26206 that this method is ambiguous with another potentially useful method replace!(new, destination, source) where new is a function giving the updated value given the old value. This method is already more or less implemented with the name Base._replace!. It would be a more general version of replace!(new, A).

Admitedly, this replace!(new, destination, source) method would have quite some overlap with map! (one difference being that replace! accepts a count keyword).

We could decide to remove conservatively replace!(pred, A, new) so that we have more time to think about this problem.
replace! was introduced in 0.7, so it would not be (strongly) breaking.

I don't have a clear opinion on this; marking for triage.

@rfourquet rfourquet added the triage This should be discussed on a triage call label Jul 5, 2018
@JeffBezanson
Copy link
Member

I agree it's too confusing to have one method that takes a predicate, and another that takes a mapping function.

@JeffBezanson
Copy link
Member

But, replace (no bang) also has this method.

@JeffBezanson
Copy link
Member

At this point in the release, triage wants to minimize disruptions.

@JeffBezanson JeffBezanson removed the triage This should be discussed on a triage call label Jul 5, 2018
@rfourquet
Copy link
Member Author

But, replace (no bang) also has this method.

Yes I wanted to mention this one but forgot. It's not needed to remove it for the possible ambiguity with another method, but maybe for consistency or if we remove the predicate methods because "it's too confusing to have one method that takes a predicate, and another that takes a mapping function."

At this point in the release, triage wants to minimize disruptions.

I understand; as it was not available on 0.6, I thought it would hopefully be not too disruptive.

@rfourquet rfourquet closed this Jul 6, 2018
@rfourquet
Copy link
Member Author

@JeffBezanson Can you confirm that triage decided against this change? Your previous message was not so explicit, and I see breaking changes still being done when this one would be practically non-breaking given it supresses a function introduced only in 0.7.

@JeffBezanson
Copy link
Member

Yes, triage decided against it, but I see now that it's just deleting something added only during 0.7, so deleting it might actually be the safer thing to do.

@rfourquet
Copy link
Member Author

Ok, so I deleted also the non-mutating function for consistency. After thinking a bit more, I still think also that this is the safer thing to do. I'm a bit sad to see the functionality go away (for the non-mutating one, we even loose a bit of non-trivial functionality, e.g. replace(iseven, [1, 2], missing) is able to infer a return type of Union{Int,Missing}, which replace(x->iseven(x) ? missing : x, [1, 2]) is not), but

  1. as said before, there may be conflicting signature for a possible future method (in the mutating case)
  2. the slightly different methods taking a function as first argument can be confusing
  3. there may be a better way; a (breaking) possibility would be for example, in the spirit of the string replace method, to have replace([1, 2], iseven => missing) (this would not allow replacing functions in an array, unless with a special wrapping, e.g. replace(Function[iseven], Some(iseven) => isodd))

@rfourquet
Copy link
Member Author

Someone feeling like merging?

@ararslan ararslan mentioned this pull request Jul 29, 2018
13 tasks
@JeffBezanson JeffBezanson merged commit 60b8e23 into master Jul 29, 2018
@JeffBezanson JeffBezanson deleted the rf/less-replace branch July 29, 2018 23:03
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.

2 participants