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

Enable wrapIdentifier for SQLite .hasTable #4915

Merged
merged 10 commits into from Jan 10, 2022

Conversation

ramos-ph
Copy link
Contributor

@ramos-ph ramos-ph commented Dec 30, 2021

Currently, the wrapIdentifier option does not apply for SQLite db.schema.hasTable.

For example, even if we set wrapIdentifier to parse identifiers to snake case, we'd get the following:

db.schema.hasTable('infoBoxes').toString()
> select * from sqlite_master where type = 'table' and name = 'infoBoxes'

This PR fixes this by enabling the wrap for SQLite Compiler:

db.schema.hasTable('infoBoxes').toString()
> select * from sqlite_master where type = 'table' and name = 'info_boxes'

Fix #4898

@OlivierCavadenti
Copy link
Collaborator

Can you add tests for this plz ?

@ramos-ph ramos-ph changed the title Enable wrapIdentifier for schema.hasTable Enable wrapIdentifier for SQLite .hasTable Jan 3, 2022
@ramos-ph
Copy link
Contributor Author

ramos-ph commented Jan 5, 2022

Hey, I came across a few bug on the test file:

describe('sqlite only', () => {
  if (!isSQLite(knex)) {
    return Promise.resolve();
  }

This will never work because the if statement runs before the describe which causes knex to be undefined - never returning true on the function output.
I double checked other workflows to make sure it wasn't a problem with my branch.
Should a new issue be considered?

@kibertoad
Copy link
Collaborator

Should a new issue be considered?

Yes, please! Could you create one?

@ramos-ph
Copy link
Contributor Author

ramos-ph commented Jan 6, 2022

Yes, please! Could you create one?

Sure! Leave it to me.

@kibertoad
Copy link
Collaborator

@OlivierCavadenti Could you review this?

@OlivierCavadenti OlivierCavadenti merged commit 40eef5c into knex:master Jan 10, 2022
@kibertoad
Copy link
Collaborator

Released in 1.0.1.

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.

hasTable method doesn't transform the name into snake_case
4 participants