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

Odd parameter names of setnames #4041

Closed
Kodiologist opened this issue Nov 12, 2019 · 5 comments · Fixed by #4043
Closed

Odd parameter names of setnames #4041

Kodiologist opened this issue Nov 12, 2019 · 5 comments · Fixed by #4043
Milestone

Comments

@Kodiologist
Copy link

@Kodiologist Kodiologist commented Nov 12, 2019

If you call setnames with just a vector of new names, then it looks like setnames(df, c("date", "place", "value")), which is fine. But if you name the argument supplying the new names, then the call looks like setnames(old = c("date", "place", "value"), df), which is confusing because you're supplying new names, not old ones. Consider allowing old to be missing, so new can be set by name instead.

@MichaelChirico
Copy link
Member

@MichaelChirico MichaelChirico commented Nov 12, 2019

The goal of the current design is to allow both usages:

iris = data.table(iris)
setnames(iris, letters[1:6])
setnames(iris, letters[1:3], LETTERS[1:3])

Unfortunately unless I'm missing something (very possible), that by necessity means the second argument will be overloaded -- in the first case, it's the new names; in the second case, it's the old names. We chose old as the argument name for clarity in the latter use case.

If you have an alternative in mind, that would be great. Or, if you explain what's wrong for your use case with using implicit argument matching, that would also be helpful.

@Kodiologist
Copy link
Author

@Kodiologist Kodiologist commented Nov 12, 2019

You should be able keep the existing overloading with an approach like the following. Keep the order and names of parameters as they are, but put this near the top of the definition of setnames:

if (is.missing(old) && !is.missing(new)) {
    old = new
    new = NULL
}

This way, all existing code should work unaltered, and it will be possible to say setnames(iris, new = LETTERS[1:6]).

@MichaelChirico
Copy link
Member

@MichaelChirico MichaelChirico commented Nov 13, 2019

@Kodiologist
Copy link
Author

@Kodiologist Kodiologist commented Nov 13, 2019

I don't follow. Since the parameter order stays the same, setnames(iris, letters[1:6]) and setnames(iris, letters[1:3], LETTERS[1:3]) will still do what they did before. Since the parameter names stay the same, any calls that used names will still work, too. The only change should be to calls that weren't legal originally, such as setnames(iris, new = LETTERS[1:6]).

@MichaelChirico
Copy link
Member

@MichaelChirico MichaelChirico commented Nov 13, 2019

maybe it's best to file a PR since it seems like you have something in mind already for how it would work.

@jangorecki jangorecki added this to the 1.12.7 milestone Nov 29, 2019
@mattdowle mattdowle removed this from the 1.12.7 milestone Dec 8, 2019
@mattdowle mattdowle added this to the 1.12.9 milestone Dec 8, 2019
@jangorecki jangorecki removed this from the 1.12.11 milestone May 26, 2020
@jangorecki jangorecki added this to the 1.12.9 milestone May 26, 2020
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 a pull request may close this issue.

4 participants