473 na strings

[jmbarbone]

resolves #473

A much cleaner PR. Nothing in the tests has to be changed for this to work.

Documentation would need to be updated if we wanted to go through with this. Happy to wait until a later release.

• add na_strings() and is_na_strings()
• implement na_strings()
• redoc

[JanMarvin]

Sorry, at my first coffee this morning, will this modify the behavior aside from replacing missing? Aka if I want to write "N/A", do I have to write na_strings("N/A")?

Or is this just something like our flavor of rlang::missing_arg()?

[jmbarbone]

Sorry, at my first coffee this morning, will this modify the behavior aside from replacing missing? Aka if I want to write "N/A", do I have to write na_strings("N/A")?

Or is this just something like our flavor of rlang::missing_arg()?

More rlang::missing_arg() flavor. Would it make sense to allow something like na_strings("N/A") or na_strings("MY_NA_VALUE") to write into the cell? Default would then be na_strings("#N/A") and that "#N/A" value is directly placed into the cell.

openxlsx2/R/write.R

Lines 311 to 330 in 2a8dbaf

	## replace NA, NaN, and Inf
	is_na <- which(cc$is == "<is><t>_openxlsx_NA</t></is>" | cc$v == "NA")
	if (length(is_na)) {
	if (is_na_strings(na.strings)) {
	cc[is_na, "v"] <- "#N/A"
	cc[is_na, "c_t"] <- "e"
	cc[is_na, "is"] <- ""
	} else {
	cc[is_na, "v"] <- ""
	if (is.null(na.strings)) {
	cc[is_na, "c_t"] <- ""
	cc[is_na, "is"] <- ""
	# do nothing
	} else {
	cc[is_na, "c_t"] <- "inlineStr"
	cc[is_na, "is"] <- txt_to_is(as.character(na.strings),
	no_escapes = TRUE, raw = TRUE)
	}
	}
	}

[JanMarvin]

Thanks, it was just for my understanding. I'll review it after the release. Should be harmless and I prefer it the way it is, but I don't have the time and focus right now. One thing at the time :)

[JanMarvin]

• Since na_strings() will be visible to the user, we should have a manual entry.
• Minor lintr issues
• Maybe update NEWS for the argument change?
• Add this as a default for wb_to_df() as well? So that the user has a clean interface? Something like wb_to_df(..., na.strings = na_strings()) { ..., if (is_na_strings) na.strings <- "#N/A").

[JanMarvin]

Looks fine to me. What about my previous comment:

* Add this as a default for `wb_to_df()` as well? So that the user has a clean interface? Something like `wb_to_df(..., na.strings = na_strings()) { ..., if (is_na_strings) na.strings <- "#N/A")`.

[JanMarvin]

@jmbarbone Please let me know what you intend to do with these patches. I like this na_strings() approach (I was actually thinking that something like a more general way like rlang::missing_arg() might even be preferable), but I personally can live without both.
So if you want it, I'll help where I can, but you'll have to update and prepare both. If you are no longer interested, I would close this and #471.

[jmbarbone]

@JanMarvin , sorry, lost track of this. I've made a few more edits and it looks like everything is running fine after merging in main.

[JanMarvin]

Thanks, I'll have a look tomorrow.

[JanMarvin]

Thank you, I have merged this PR. Looks nice :)

But please, before you start updating #471, I still haven't made up my mind about the size and impact of the PR and would veto the merge at this point. I still feel that the risk of breaking things along the way still outweighs the benefit of having more organized (and possibly cleaner) code. Since I started porting some real life work over to openxlsx2 (which exposed some of the bugs I have been fixing lately), I don't like having a nice but broken mess and prefer a working mess, if you know what I mean.

Maybe we can break #471 into smaller pieces (e.g. not including the unnecessary testthat changes) or go function by function, but just having one big pull request that changes 61 files and moves ~5k lines of code is not what I like at the moment. And even if it does not make things better, I am really sorry to be so picky about this, especially since you have already spent some time on it.