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 test to assert that table and column names are lowercase #46

Merged
merged 5 commits into from
Jun 6, 2023

Conversation

ginberg
Copy link
Collaborator

@ginberg ginberg commented May 30, 2023

see #44

@ginberg ginberg requested a review from fdefalco May 30, 2023 08:40
@ginberg ginberg linked an issue Jun 1, 2023 that may be closed by this pull request
@@ -30,6 +30,17 @@ test_that("Connect", {
DatabaseConnector::disconnect(connection)
})

test_that("Table names and column names case", {
connection <- DatabaseConnector::connect(getEunomiaConnectionDetails())
tableNames <- DatabaseConnector::getTableNames(connection)
Copy link
Collaborator

Choose a reason for hiding this comment

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

tableNames <- DatabaseConnector::getTableNames(connection, cast = "none")

Perhaps you should suppress the casting that is done by getTableNames to ensure the columns are actually lowercase in the database. I'd probably use DBI for this but DatabaseConnector::getTableNames should work.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ablack3 that's a good point about the cast argument, I didn't know that. I saw that you have added the parameter quite recently and so it's only available in DatabaseConnector starting from 6.0.0. Should we therefore increase the minimum version of DatabaseConnector in the DESCRIPTION file?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ablack3 I have updated the test

…ector since from that version table names have been made lowercase
@ablack3
Copy link
Collaborator

ablack3 commented Jun 6, 2023

Looks good to me @ginberg! Thanks for this contribution.

@fdefalco fdefalco merged commit ae18eb0 into develop Jun 6, 2023
@fdefalco fdefalco deleted the eu44 branch June 6, 2023 15:29
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.

Add tests to assert that column names are lowercase
3 participants