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
Collaborator

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();
Copy link
Collaborator

Choose a reason for hiding this comment

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

are those fields not used anywhere?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I didn't found any usages in project

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fair enough, guess we can drop it.

@kibertoad
Copy link
Collaborator

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

@maximelkin
Copy link
Collaborator Author

Ok, i will resolve it later

@maximelkin maximelkin force-pushed the bluebird_remove_bind branch 2 times, most recently from b5d15cf to 9382295 Compare October 14, 2019 20:39
@@ -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();
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

_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.
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

@kibertoad
Copy link
Collaborator

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

@maximelkin
Copy link
Collaborator Author

@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
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

3 participants