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

Add several conversion, string, and logic functions (mostly from MS SQL Server) #26

Merged
merged 15 commits into from
Mar 29, 2020

Conversation

StevenHibble
Copy link
Contributor

This is a loose collection of additions to the translations. There is one fix that I noticed while looking for missing function (nullif's base R translation). Here's what is added:

  1. Conversion functions from SQL Server (TRY_CAST, CONVERT, and TRY_CONVERT) to complement CAST
  2. String functions (STRING_AGG, REVERSE, REPLACE, CHARINDEX, and REPLICATE)
  3. Logic functions (ISNULL, NVL, CHOOSE)

@StevenHibble
Copy link
Contributor Author

It looks like there aren't tests for translations in general. I'm not sure how that would work, but I'd be happy to provide some.

@ianmcook
Copy link
Owner

ianmcook commented Feb 6, 2020

Awesome, thank you @StevenHibble! I will take a closer look at this PR shortly.

Correct, I haven't yet added comprehensive tests of the function translations. If you'd like to give that a go, please do. You could add a test script named test-translations.R and create tests like the ones in test-parse_expression.R, calling parse_expression() but in a way that isolates specific function translations as much as possible. Or if you have ideas for how to do it better or more efficiently, go for it. Thanks!

@ianmcook
Copy link
Owner

ianmcook commented Feb 6, 2020

P.S. The main reason that the codecov test coverage is not at or near 100% is that many of the function translations are currently not tested!

R/replace.R Outdated Show resolved Hide resolved
R/translations.R Outdated Show resolved Hide resolved
R/translations.R Outdated Show resolved Hide resolved
R/translations.R Outdated Show resolved Hide resolved
Copy link
Owner

@ianmcook ianmcook left a comment

Choose a reason for hiding this comment

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

This looks great—thank you! I have just a few small change requests, and some questions and general discussion—please take a look at my comments. I'll wait to merge until after you push commits to address those (they should automatically get added to the PR).

@StevenHibble
Copy link
Contributor Author

I'll try to make these changes over the weekend.

@ianmcook
Copy link
Owner

ianmcook commented Feb 7, 2020

Thanks! And please don't feel any obligation to rush 🙂

@StevenHibble
Copy link
Contributor Author

R CMD check did throw this warning:

checking for unstated dependencies in ‘tests’ ... WARNING
'::' or ':::' import not declared from: ‘dplyr’

@StevenHibble StevenHibble reopened this Feb 8, 2020
@ianmcook
Copy link
Owner

ianmcook commented Feb 9, 2020

To stop those unstated dependencies warnings, you'll need to call tidyverse functions by using str2lang(), just like you did in the translations you added for charindex() and replace().

@StevenHibble
Copy link
Contributor Author

Thanks for the guidance. The references were in tests added for the NULLS FIRST/LAST commit.

@ianmcook
Copy link
Owner

Thank you @StevenHibble! I will review soon.

@ianmcook ianmcook merged commit 7f34901 into ianmcook:master Mar 29, 2020
@ianmcook
Copy link
Owner

Sorry for the epic delay in merging this. Thanks again for your work on this!

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