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

refactor(bluebird): remove Bluebird.bind #3477

Merged
merged 4 commits into from Oct 15, 2019

Conversation

@maximelkin
Copy link
Contributor

maximelkin commented Oct 12, 2019

No description provided.

@@ -475,7 +474,7 @@ class Migrator {
_waterfallBatch(batchNo, migrations, direction, trx) {
const trxOrKnex = trx || this.knex;
const { tableName, schemaName, disableTransactions } = this.config;
let current = Bluebird.bind({ failed: false, failedOn: 0 });
let current = Promise.resolve();

This comment has been minimized.

Copy link
@kibertoad

kibertoad Oct 12, 2019

Collaborator

are those fields not used anywhere?

This comment has been minimized.

Copy link
@maximelkin

maximelkin Oct 12, 2019

Author Contributor

I didn't found any usages in project

This comment has been minimized.

Copy link
@maximelkin

maximelkin Oct 12, 2019

Author Contributor

7a6937f first revision where failedOn exists, i think it just partially implemented feature

This comment has been minimized.

Copy link
@kibertoad

kibertoad Oct 13, 2019

Collaborator

Fair enough, guess we can drop it.

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Oct 13, 2019

Sorry, there is a merge conflict now after refactoring of a seeder. Could you resolve them?

@maximelkin

This comment has been minimized.

Copy link
Contributor Author

maximelkin commented Oct 13, 2019

Ok, i will resolve it later

@maximelkin maximelkin force-pushed the maximelkin:bluebird_remove_bind branch 2 times, most recently from b5d15cf to 9382295 Oct 14, 2019
@@ -28,7 +20,7 @@ class Seeder {
// Runs seed files for the given environment.
async run(config) {
this.config = this.setConfig(config);
const [all] = await this._seedData();
const all = await this._listAll();

This comment has been minimized.

Copy link
@kibertoad

kibertoad Oct 15, 2019

Collaborator

Will types changed in https://github.com/knex/knex/pull/3438/files still be correct after this change?

This comment has been minimized.

Copy link
@maximelkin

maximelkin Oct 15, 2019

Author Contributor

_seedData is just _listAll, but wrapping result inside promise into []

@@ -103,22 +85,14 @@ class Seeder {
);
}

// Generates the stub template for the current seed file, returning a compiled template.

This comment has been minimized.

Copy link
@kibertoad

kibertoad Oct 15, 2019

Collaborator

@lorefnon I assume we stopped using it after your refactoring?

This comment has been minimized.

Copy link
@lorefnon

lorefnon Oct 15, 2019

Collaborator

Yeah, i moved the lodash template usage to a shared utility which we can potentially use from other places as well.

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Oct 15, 2019

@maximelkin Merged your other PR, that seems to have introduced a minor conflict as well.

@maximelkin maximelkin force-pushed the maximelkin:bluebird_remove_bind branch from 5886a20 to 5e9343e Oct 15, 2019
@maximelkin

This comment has been minimized.

Copy link
Contributor Author

maximelkin commented Oct 15, 2019

@kibertoad done

@maximelkin maximelkin closed this Oct 15, 2019
@maximelkin maximelkin reopened this Oct 15, 2019
@maximelkin maximelkin closed this Oct 15, 2019
@maximelkin maximelkin reopened this Oct 15, 2019
@kibertoad kibertoad merged commit d01600b into knex:master Oct 15, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
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.