knitr icon indicating copy to clipboard operation
knitr copied to clipboard

added options immedate & replace to eng_SQL

Open AWKruijt opened this issue 2 years ago • 6 comments

As discussed here: fixes https://github.com/rstudio/rmarkdown/issues/2241

I haven't added test files as these changes require access to an sql server with permission to create temporay tables. Haven't found that in an open database.

I'm excited about this! :)

AWKruijt avatar May 18 '22 09:05 AWKruijt

reptable = gsub('(^.into[[:space:]]+)(#.+)([[:space:]]+from.$)', '\\2', sql, ignore.case = T) on line 618 fixes the error handling on attempts to replace non-temporary tables.

AWKruijt avatar May 18 '22 09:05 AWKruijt

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Anne-Wil seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

CLAassistant avatar May 18 '22 09:05 CLAassistant

Just checking (I'm really inexperienced >.<): I'm seeing this super inviting 'Update branch' button (with alternate update with rebase) - do I push that or do I just wait now? :)

AWKruijt avatar May 18 '22 10:05 AWKruijt

You can push that to update but that is not necessary. We'll do it ourself, or just merge the branch. Your change target in code part that has not been modified in some time, and tests are passing ok. So you can leave that way, and just wait for us to review. I added that in my TODO list. Thanks !

cderv avatar May 18 '22 10:05 cderv

Sorry for the radio silence - things where happening. Thanks for all the input! I've implemented as much as I could - see commit 358d552.

A big one that is open for debate still is whether/how to make it so that paramater immediate is NOT passed to dbGetQuery when the user did not set it. I suppose that's what we want, given that it's cleanest if we also don't pass the NULL value if sql.immediate is not actively invoked. For option params the code has a split based on length(params) == 0 and separately defined calls to DBI::dbGetQuery() with and without passing of params. We could expand that into four calls (with/without immediate and/or params), but perhaps something along the lines of a do.call with a list of arguments could be preferable? I did try every trick I could think of along the lines of DBI::dbGetQuery(conn, sql, if(exists('immedate'){immedate = immediate}) but no joy :/

AWKruijt avatar Jun 14 '22 10:06 AWKruijt

Alas, I notice that in practice the replace option doesn't always work flawlessly due to the regex to lift the tempary table name from the query also picking up new lines (/n) immediately after the #table name. Fixed the regex and made another correction to the getQuery call with params where I had made it use sql instead of query.

AWKruijt avatar Jun 24 '22 09:06 AWKruijt