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

Separate migration generator #2786

Merged
merged 1 commit into from Sep 17, 2018

Conversation

Projects
None yet
4 participants
@JakeGinnivan
Copy link
Contributor

JakeGinnivan commented Aug 26, 2018

This actually builds on top of my other PR, but didn't want to do this refactor in that PR.

Seeing as Migrator is no longer tied to the filesystem, I thought I would pull the make() function and all supporting helpers into it's own class. Will make it easier to ensure no fs related dependencies sneak back into Migrator.

This is just an idea, happy for it to be closed if you folks don't think it's a good idea :) It's just the last commit which is new in addition to the last PR - 76ebc1e

import fs from 'fs';
import path from 'path';
import mkdirp from 'mkdirp';
import Promise from 'bluebird';

This comment has been minimized.

@kibertoad

kibertoad Aug 26, 2018

Collaborator

Can this be replaced with native Promise?

This comment has been minimized.

@JakeGinnivan

JakeGinnivan Aug 26, 2018

Contributor

When I did that, linting failed. Thoughts?

This comment has been minimized.

@wubzz

wubzz Aug 26, 2018

Collaborator

There should be no need to focus on using native promises since knex already depends heavily on bluebird.

There's been suggestions before to not have this dependency - or at the very least the option to supply promise lib/native - but I believe until such a decision is made, it's fine to keep using bluebird.

This comment has been minimized.

@kibertoad

kibertoad Aug 26, 2018

Collaborator

@wubzz
#1588 -> there is very clearly communicated intent to eventually go away off using bluebird, hence I see why not to avert using it in newly touched code.

@JakeGinnivan What was the linting error?

This comment has been minimized.

@kibertoad

kibertoad Aug 26, 2018

Collaborator

@JakeGinnivan Ah, most likely linting error was due to use of Promise.promisify -> you can't just remove reference to Promise here, instead import promisify explicitly.

This comment has been minimized.

@wubzz

wubzz Aug 26, 2018

Collaborator

@kibertoad Afaik that comment is strictly in regards to the "utility" functions exposed by knex (such as .map etc) and TS typings, not removing bluebird as a whole. Especially not as a first step.

In any case it's not on the board for the next version of knex and certainly not set in stone. Contributors should not have to take this potential future change into consideration when developing features in present time, imho.

This comment has been minimized.

@JakeGinnivan

JakeGinnivan Aug 27, 2018

Contributor

Linting error was Promise is not defined

path.join(__dirname, 'stub', this.config.extension + '.stub');

return Promise.promisify(fs.readFile, { context: fs })(stubPath).then(
(stub) => template(stub.toString(), { variable: 'd' })

This comment has been minimized.

@kibertoad

kibertoad Aug 26, 2018

Collaborator

Can this be rewritten using string template?

@@ -376,7 +345,7 @@ export default class Migrator {
);
})
.then(() => {
log.push(path.join(directory, name));
log.push(name);

This comment has been minimized.

@kibertoad

kibertoad Aug 26, 2018

Collaborator

Why?

This comment has been minimized.

@JakeGinnivan

JakeGinnivan Aug 26, 2018

Contributor

This change is in the other PR, mainly because I don't have this info anymore. And I can't add the migration directory into the migration name.

I could add a getMigrationId() vs getMigrationName(). Id would return what is stored in the knex migration table, and the name could be logged / used for diagnostics.

Thoughts?

// are used
if (
config &&
!config.migrationSource &&

This comment has been minimized.

@kibertoad

kibertoad Aug 26, 2018

Collaborator

Emm, migration source new functionality leaked into refactoring PR?..

This comment has been minimized.

@JakeGinnivan

JakeGinnivan Aug 26, 2018

Contributor

This one is based on the other one. To save rebasing, these changes will go away once the other one is merged. If you just look at the last commit thats the only new one

@elhigu

This comment has been minimized.

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Sep 12, 2018

@elhigu Probably makes sense to ignore this one for now, it is closely related to #2775 and probably should be reassessed after that one lands.

@JakeGinnivan

This comment has been minimized.

Copy link
Contributor

JakeGinnivan commented Sep 13, 2018

Yeah, I will fix this one up now the other PR has been merged. Want me to close for now until I do that?

@elhigu

This comment has been minimized.

Copy link
Collaborator

elhigu commented Sep 13, 2018

No need to close, take your time 👍

@JakeGinnivan JakeGinnivan force-pushed the JakeGinnivan:separate-migration-generator branch 2 times, most recently from 86575a1 to bf21d10 Sep 17, 2018

@JakeGinnivan

This comment has been minimized.

Copy link
Contributor

JakeGinnivan commented Sep 17, 2018

@elhigu I have updated the PR. the coverage has decreased, not really sure why, thoughts on what tests I should add?

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Sep 17, 2018

@JakeGinnivan I don't think issue is with your code, see #2810 -> after it is merged, I think that rerunning CI on PR should pass.

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Sep 17, 2018

@JakeGinnivan Please pull changes from tgriesser/knex master, that should resolve the issue

@JakeGinnivan JakeGinnivan force-pushed the JakeGinnivan:separate-migration-generator branch from bf21d10 to 80a4b53 Sep 17, 2018

@JakeGinnivan

This comment has been minimized.

Copy link
Contributor

JakeGinnivan commented Sep 17, 2018

@kibertoad rebased

@JakeGinnivan

This comment has been minimized.

Copy link
Contributor

JakeGinnivan commented Sep 17, 2018

Also, if you don't agree with separating these classes, happy for this one to just close. Is just an idea

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Sep 17, 2018

Personally, I think that this is very much a step in the right direction.
Huh, coverage calculation is still off. Wth?..

@elhigu

This comment has been minimized.

Copy link
Collaborator

elhigu commented Sep 17, 2018

We shouldn't care that much about coverage... those calculations did seem pretty strange like there were some files, where it wasn't able to calculate any coverages.

@elhigu elhigu merged commit d0e3be4 into tgriesser:master Sep 17, 2018

1 of 2 checks passed

coverage/coveralls Coverage decreased (-2.0%) to 83.698%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@kibertoad kibertoad deleted the JakeGinnivan:separate-migration-generator branch Sep 17, 2018

@JakeGinnivan

This comment has been minimized.

Copy link
Contributor

JakeGinnivan commented Sep 18, 2018

Any chance of a @next tagged release so I can give all this a spin :)

@elhigu

This comment has been minimized.

Copy link
Collaborator

elhigu commented Sep 18, 2018

yep, it is coming I already wrote change logs last night for it.

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Sep 18, 2018

@elhigu Would be great to also include #2802 in @next release to unblock people waiting for it :)

@elhigu

This comment has been minimized.

Copy link
Collaborator

elhigu commented Sep 18, 2018

Everything that is in master is going to be there. I also document how to do the next release so that people can easily update it to have code from latest master.

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Sep 18, 2018

#2802 is not in master yet, hence what I mean is that it would be great to release next after #2802 is approved and merged :)

@elhigu

This comment has been minimized.

Copy link
Collaborator

elhigu commented Sep 18, 2018

or afterwards and do another release, I don't want to rush merging it before manual testing and that is probably more time consuming than finishing the next release for current master.

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Sep 18, 2018

Or that :)

@JakeGinnivan

This comment has been minimized.

Copy link
Contributor

JakeGinnivan commented Sep 18, 2018

Awesome! Thanks heaps :)

@elhigu

This comment has been minimized.

Copy link
Collaborator

elhigu commented Sep 18, 2018

And knex@next is now available.

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Sep 18, 2018

There goes my sleep 😂
runs off to add shiny new stuff to work project

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment