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

Closes #1320 #1322

Closed
wants to merge 1 commit into from
Closed

Closes #1320 #1322

wants to merge 1 commit into from

Conversation

MichaelChirico
Copy link
Member

adds rownames argument to setDF, per #1320; adds an error for the case that rownames is given of incorrect length.

@jangorecki
Copy link
Member

Looks pretty fine. Some minor things that can be added is a more verbose error message on non-matching length - there could be info about it should match to nrow - and a check if rownames is character as I believe data.frames rownames must be a character(?).

edit: before PR be sure to install new version locally and run test.data.table(). I restart R session each time I install data.table just to be sure it won't cause issues.

@MichaelChirico
Copy link
Member Author

@jangorecki thanks for the quick feedback. my first more-than-trivial commit on a project that's not mine :)

As to the error message, I think that error is handled "on the backend", e.g.:

DF <- data.frame(a=1:10,b=1:10)
setDF(DF, rownames = factor(LETTERS[1:10]))

gives me an error:

Error in setattr(x, "row.names", rownames) : 
  row names must be 'character' or 'integer', not 'integer'
Called from: setattr(x, "row.names", rownames)

More worrying is that there's no such "free lunch" with respect to duplicate names, so I think I'll add a check for that: setDF(DF, rownames = rep("a",10)) returns no error until the next time you try and display DF when it gives: Error in data.frame(a = c(" 1", " 2", " 3", " 4", " 5", " 6", " 7", " 8", : duplicate row.names: A

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.

None yet

2 participants