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 replaceIf or setAt #65

Closed
pzp1997 opened this issue Jun 23, 2017 · 8 comments
Closed

Rename replaceIf or setAt #65

pzp1997 opened this issue Jun 23, 2017 · 8 comments

Comments

@pzp1997
Copy link
Contributor

pzp1997 commented Jun 23, 2017

replaceIf and setAt are really two variants of the same kind of operation. They both replace some element(s) with a given value. Perhaps their naming should reflect this similarity. (Similar to how we have updateIf and updateAt.) Some questions to consider.

  1. Are there any advantages provided by keeping the current naming?
  2. If not, which name describes the operation better? (It may or may not be relevant to the discussion to note that Array uses the name set for a similar operation.)
  3. Perhaps out of the scope of this issue but also related, do we even need setAt and replaceIf when they are essentially equivalent to calling updateAt and updateIf with an update function of always x.
@Chadtech
Copy link
Collaborator

Answering question one, we've already made some major changes, so I think now is the time to cohere the names of set, replace, and update; otherwise I would be more hesitant. Maybe we should rename replaceIf to setIf.

I do think update has independent value from set and replace. Set and replace take a distinct value, where as update is agnostic on what value its updating. Without update, you have to get the value, and then replace it.

@pzp1997
Copy link
Contributor Author

pzp1997 commented Jun 24, 2017

I agree with you about the timing due to having already made breaking changes for the next version. Regarding # 3, I was actually suggesting that we get rid of replaceIf and setAt. It is quite simple for a user to replace their behavior using updateIf and updateAt.

@Chadtech
Copy link
Collaborator

Like if they want to replace, they should do something like updateAt 0 (always 1) [ 0, 1, 2 ]?

@pzp1997
Copy link
Contributor Author

pzp1997 commented Jun 26, 2017

Correct, that's all we would be doing in our implementation anyway. (Currently the implementation for setAt is more complicated, but that should change once I find the time to finish #55.) The convenience it provides is pretty minimal.

One point in favor of keeping it would be that using always might be non-obvious to some people (but I do not think that this is the case for the vast majority of Elm developers). It is already here and I suppose we have very little to lose by keeping it.

I'm somewhat indifferent on the matter of removing it, but I do think that if we choose to keep it either replaceIf should be renamed to setIf or setAt should be renamed to replaceAt.

@Chadtech
Copy link
Collaborator

Chadtech commented Jul 1, 2017

I think keeping it is a much better option. Its conceptually simplest that their would be different functions for setting, updating, and replacing, even if a little bit of reasoning can show that an update is a kind of replace. More functions necessary to execute an update (as is the case in always 1, make things less readable too.

@pzp1997
Copy link
Contributor Author

pzp1997 commented Jul 2, 2017

Now that I've had some time to think about it, I agree with you. My vote is in favor of renaming replaceIf to setIf then.

@pzp1997
Copy link
Contributor Author

pzp1997 commented Sep 1, 2017

@Chadtech Let me know if you want a PR for this.

@Chadtech
Copy link
Collaborator

Closing since we followed through on this.

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

No branches or pull requests

2 participants