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

read_excel now accepts a vector of NA types #136

Closed
wants to merge 1 commit into from

Conversation

MichaelChirico
Copy link
Contributor

Can be tested on this test Excel file:

https://drive.google.com/file/d/0B1i6iIc4UbO1VUV6TWQ0RjlFT1U/view?usp=sharing

Using this code:

read_excel("test_mult_na.xlsx",sheet=1,col_names=T,
           col_types=c("text","numeric","numeric"),na=c("","-","NA"))

I get this output, as expected:

  var1     var2 var3
1    a    1.300    1
2    b       NA    2
3    c   51.500    3
4 <NA>    9.900    4
5    e 8548.400    5
6    f  848.560    6
7    g   32.498   NA

pass number 2

pass number 3

pass number 4

pass number 5

pass number 6

pass number 7

pass number 8

pass number 9

pass number 10

Finished source edit, appears to work; updated man page for read_excel
@MichaelChirico
Copy link
Contributor Author

Not sure why Travis / R CMD check --as-cran failed, as the latter works on my local machine.

@@ -48,13 +48,13 @@ inline std::string cellTypeDesc(CellType type) {

inline CellType cellType(xls::st_cell::st_cell_data cell, xls::st_xf* styles,
const std::set<int>& customDateFormats,
std::string na = "") {
std::vector<std::string> na = {""}) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That needs to be a const std::vector<std::string>& otherwise the complete vector gets copied each time

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that default value a C++11 extension?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hadley Thanks for the tip! This is honestly my first piece of C++ code so I can't say I have the slightest idea which bits are 11-specific or not.

Before repairing this pull, I noticed after submitting this request that there's an outstanding pull request that seems to accomplish the same thing and from what I see was just about ready to be merged, was there something wrong with that code that I missed?

Don't want to duplicate efforts here.

@hadley
Copy link
Member

hadley commented Jan 18, 2017

Closed as duplicate of #56

@hadley hadley closed this Jan 18, 2017
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