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

75 add data #166

Merged
merged 8 commits into from
May 9, 2022
Merged

75 add data #166

merged 8 commits into from
May 9, 2022

Conversation

jmbarbone
Copy link
Collaborator

related to #75

I didn't do anything to the actual write_data() and write_datatable() functions. These aren't incorporated into the R6 object, but instead called within them and a wrapper created to call that. This means we can keep working off the write_*() functions until they're reworked to be called directly from the workbook.

I'm also thinking about if it makes sense to use $add_data_table() or just change that to $add_table(). That would be an easy change.

@JanMarvin
Copy link
Owner

Looks good, I'll make a run tomorrow. Not sure about calling it add_data(), write seems more natural, but that's your decision and I understand your augmentation.

@jmbarbone
Copy link
Collaborator Author

Not sure about calling it add_data(), write seems more natural, but that's your decision and I understand your augmentation.

Technically I guess we're writing everything because it's modifying the xml? This does at least keep it consistent with the other add_/get_/set_ etc prefixes.

@JanMarvin
Copy link
Owner

As I said, I understand your reason, but whenever one processes data in R for the purpose of storing it, the functions that do this are called write.... So we sacrifice principles of the language for consistency, I just wanted to note that. On the other hand, dbplyr also does this with copy_to(), so probably I'm just being too intellectually inflexible.

I've updated the README.

@JanMarvin JanMarvin merged commit 7831471 into main May 9, 2022
@JanMarvin JanMarvin deleted the 75-add-data branch October 27, 2022 23:13
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