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

fix: migrate describeTableQuery and showIndexesQuery to TS #15299

Merged
merged 19 commits into from Nov 20, 2022

Conversation

WikiRik
Copy link
Member

@WikiRik WikiRik commented Nov 18, 2022

Pull Request Checklist

  • Have you added new tests to prevent regressions?
  • If a documentation update is necessary, have you opened a PR to the documentation repository?
  • Did you update the typescript typings accordingly (if applicable)?
  • Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
  • Does the name of your PR follow our conventions?

Description Of Change

This PR builds on the new extractTableDetails to provide a cleaner describeTableQuery function. Schemas are now tested for all dialects. There is one behaviour change since the previous PR, which is that the schema is only used in the query when it's not the defaultSchema of the dialect. I would like that to be a choice we make throughout the codebase consistently. That is in line with the changes to quoteTable

It also migrates showIndexesQuery with a new test for the above mentioned behaviour of quoteTable.
It also adds that test to dropTableQuery and adds the proper infix for the quoteIdentifier test file name.

@WikiRik WikiRik requested a review from ephys November 18, 2022 08:42
@WikiRik
Copy link
Member Author

WikiRik commented Nov 18, 2022

@ephys could you already take a look at this before I do the same approach for some other queryGenerator functions?

@WikiRik
Copy link
Member Author

WikiRik commented Nov 18, 2022

I'm not sure if I fixed the failing postgres test correctly, but according to the withSchema docs it will place a schema in front. So it's strange that it didn't pick this up before. Maybe because the schema is not always specified anymore for postgres (it had a fallback to public before)?

@WikiRik
Copy link
Member Author

WikiRik commented Nov 18, 2022

I haven't migrated dropTableQuery to TS yet since it uses rejectInvalidOptions. I think it's a separate PR on how to deal with that

@WikiRik WikiRik changed the title fix: migrate various queryGenerator to TS with unified unit tests fix: migrate describeTableQuery and showIndexesQuery to TS Nov 18, 2022
@WikiRik WikiRik marked this pull request as ready for review November 18, 2022 12:39
src/dialects/db2/query-generator-typescript.ts Outdated Show resolved Hide resolved
src/dialects/mssql/query-generator-typescript.ts Outdated Show resolved Hide resolved
src/utils/deprecations.ts Outdated Show resolved Hide resolved
test/integration/model.test.js Outdated Show resolved Hide resolved
test/unit/query-generator/describe-table-query.test.ts Outdated Show resolved Hide resolved
@WikiRik WikiRik marked this pull request as draft November 18, 2022 20:02
@ephys
Copy link
Member

ephys commented Nov 19, 2022

I need to make further improvements to describeTable to be able to use it in the tests of #14687.
It's very likely that I'll drop the schema option in a follow-up PR, so I don't know about adding the deprecation notice

@WikiRik
Copy link
Member Author

WikiRik commented Nov 19, 2022

I need to make further improvements to describeTable to be able to use it in the tests of #14687. It's very likely that I'll drop the schema option in a follow-up PR, so I don't know about adding the deprecation notice

Let's still deprecate it here (but move it to describeTable), and you can remove it in a different PR if you know for sure that you'll drop it

@WikiRik
Copy link
Member Author

WikiRik commented Nov 19, 2022

I think we should be good for this PR.

Future PRs are;

  • Updating quoteTable so it always returns a schema. See fix: migrate describeTableQuery and showIndexesQuery to TS #15299 (comment)
  • Removing the default schema workaround USER from DB2 and CURRENT SCHEMA from ibmi once we know the wanted schema. (USER might be a default schema for DB2, but it requires some further investigation)
  • Discuss how to migrate methods that use rejectInvalidOptions. (preferably in a way that we do not need both a Set with possible options and the type)

@WikiRik WikiRik marked this pull request as ready for review November 19, 2022 21:59
@WikiRik WikiRik requested a review from ephys November 19, 2022 22:00
src/dialects/abstract/query-generator-typescript.ts Outdated Show resolved Hide resolved
src/dialects/abstract/query-interface.js Outdated Show resolved Hide resolved
src/dialects/abstract/query-interface.js Outdated Show resolved Hide resolved
src/dialects/abstract/query-generator-typescript.ts Outdated Show resolved Hide resolved
src/dialects/db2/query-generator-typescript.ts Outdated Show resolved Hide resolved
src/dialects/db2/query-generator-typescript.ts Outdated Show resolved Hide resolved
src/dialects/ibmi/query-generator-typescript.ts Outdated Show resolved Hide resolved
src/dialects/ibmi/query-generator-typescript.ts Outdated Show resolved Hide resolved
@WikiRik WikiRik requested a review from ephys November 19, 2022 23:40
ephys
ephys previously approved these changes Nov 19, 2022
Copy link
Member

@ephys ephys left a comment

Choose a reason for hiding this comment

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

Thanks :)

@WikiRik WikiRik merged commit e1ba925 into main Nov 20, 2022
@WikiRik WikiRik deleted the WikiRik/more-query-generator-unit-tests branch November 20, 2022 07:43
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