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 typing file #3505

Merged
merged 2 commits into from Nov 12, 2019
Merged

Fix typing file #3505

merged 2 commits into from Nov 12, 2019

Conversation

@MiOnim
Copy link
Contributor

MiOnim commented Oct 30, 2019

Two changes in the typings file:

  • Fixed the type of log method in PoolConfig interface
  • Removed create, destroy and validate methods from PoolConfig interface, because Knex is overriding these methods, and any user provided implementations of these methods are being ignored. Having these methods in the typings file will only confuse users.
MiOnim added 2 commits Oct 30, 2019
because Knex is overriding these methods, and any user provided implementation of these methods will be ignored
@MiOnim

This comment has been minimized.

Copy link
Contributor Author

MiOnim commented Nov 5, 2019

Can someone please review this? CC: @elhigu

@elhigu

This comment has been minimized.

Copy link
Member

elhigu commented Nov 5, 2019

I think that at least validate should be able to be overridden by the user. But I believe that the best way to implement pool config typings would be to somehow inherit/extend it from tarn's config type and add javascript level errors, when trying to override create/destroy. I'm not completely sure if even overriding those must be denied if the one providing new implementations knows what they are doing 🤔

@MiOnim

This comment has been minimized.

Copy link
Contributor Author

MiOnim commented Nov 6, 2019

@elhigu Knex seems to ignore any user provided implementation of validate/create/destroy, and pass its own implementation of these methods to tarn. I was just trying to update the typings file to express this intent accurately. Do you want me to change Knex's behavior and allow users to override those methods?

@elhigu

This comment has been minimized.

Copy link
Member

elhigu commented Nov 10, 2019

Do you want me to change Knex's behavior and allow users to override those methods?

No need to change the current implementation. If some one starts requesting it it will be easy to improve afterwards.

@MiOnim

This comment has been minimized.

Copy link
Contributor Author

MiOnim commented Nov 11, 2019

@elhigu In that case, this PR should be ready to be approved and merged?

@elhigu elhigu closed this Nov 12, 2019
@elhigu elhigu reopened this Nov 12, 2019
@elhigu

This comment has been minimized.

Copy link
Member

elhigu commented Nov 12, 2019

Retriggered tests... for some reason there was no button visible in travis to retry those.

@MiOnim MiOnim closed this Nov 12, 2019
@MiOnim MiOnim reopened this Nov 12, 2019
@kibertoad kibertoad merged commit 115ff3c into knex:master Nov 12, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Nov 14, 2019

Released in 0.20.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.