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

This package is preventing adding support for new SQL dialects #716

Closed
schuemie opened this issue Sep 30, 2024 · 6 comments
Closed

This package is preventing adding support for new SQL dialects #716

schuemie opened this issue Sep 30, 2024 · 6 comments

Comments

@schuemie
Copy link
Member

One of the unit tests is to see if the generated SQL files match those that were pre-generated. This is done for all dialects supported by SqlRender as defined here. We just added the 'iris' dialect to SqlRender, which of course isn't one of the pre-rendered translations.

When I push SqlRender to CRAN, because CommonDataModel depends on SqlRender its unit tests are run with the new version. Because the pre-rendered SQL files do not contain the IRIS translations the unit tests fail and CRAN rejects the new SqlRender version.

In general I think this unit test is far too restrictive. It does not allow any changes to SqlRender. Even if the list of dialects stays the same, if SqlRender comes with a different but semantically equivalent translation it is blocked by CRAN. Please remove the unit test, or make it a lot more lenient.

Adding @bdeboe and @alex-odysseus for awareness of the first of probably many consequences of adding a new dialect.

@schuemie
Copy link
Member Author

Please push a new release to CRAN ASAP. I cannot create a new SqlRender release until this is resolved. @clairblacketer @fdefalco

@bdeboe
Copy link

bdeboe commented Sep 30, 2024

I filed PR #717 adding the pregenerated DDL files for the IRIS dialect, hoping that helps.

@clairblacketer
Copy link
Contributor

thanks both. To start I will remove the offending unit tests and resubmit to CRAN. I'll let you know when it is complete

@clairblacketer
Copy link
Contributor

I resubmitted to CRAN v1.0.1 of the package

@clairblacketer
Copy link
Contributor

@schuemie v1.0.1 of the CommonDataModel package was accepted and is on its way to CRAN with the offending test removed.

@schuemie
Copy link
Member Author

schuemie commented Oct 2, 2024

Thanks!

@schuemie schuemie closed this as completed Oct 2, 2024
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

No branches or pull requests

3 participants