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

Separate migration generator #2786

Merged
merged 1 commit into from Sep 17, 2018

Conversation

@JakeGinnivan
Copy link
Contributor

@JakeGinnivan 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';
Copy link
Collaborator

@kibertoad kibertoad Aug 26, 2018

Can this be replaced with native Promise?

Copy link
Contributor Author

@JakeGinnivan JakeGinnivan Aug 26, 2018

When I did that, linting failed. Thoughts?

Copy link
Member

@wubzz wubzz Aug 26, 2018

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.

Copy link
Collaborator

@kibertoad kibertoad Aug 26, 2018

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

Copy link
Collaborator

@kibertoad kibertoad Aug 26, 2018

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

Copy link
Member

@wubzz wubzz Aug 26, 2018

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

Copy link
Contributor Author

@JakeGinnivan JakeGinnivan Aug 27, 2018

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' })
Copy link
Collaborator

@kibertoad kibertoad Aug 26, 2018

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

@kibertoad kibertoad Aug 26, 2018

Why?

Copy link
Contributor Author

@JakeGinnivan JakeGinnivan Aug 26, 2018

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 &&
Copy link
Collaborator

@kibertoad kibertoad Aug 26, 2018

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

Copy link
Contributor Author

@JakeGinnivan JakeGinnivan Aug 26, 2018

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
Copy link
Member

@elhigu elhigu commented Sep 12, 2018

@kibertoad
Copy link
Collaborator

@kibertoad 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
Copy link
Contributor Author

@JakeGinnivan 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
Copy link
Member

@elhigu elhigu commented Sep 13, 2018

No need to close, take your time 👍

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

@JakeGinnivan 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
Copy link
Collaborator

@kibertoad 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
Copy link
Collaborator

@kibertoad kibertoad commented Sep 17, 2018

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

@JakeGinnivan JakeGinnivan force-pushed the separate-migration-generator branch from bf21d10 to 80a4b53 Sep 17, 2018
@JakeGinnivan
Copy link
Contributor Author

@JakeGinnivan JakeGinnivan commented Sep 17, 2018

@kibertoad rebased

@JakeGinnivan
Copy link
Contributor Author

@JakeGinnivan 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
Copy link
Collaborator

@kibertoad 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
Copy link
Member

@elhigu 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 knex:master Sep 17, 2018
1 of 2 checks passed
@kibertoad kibertoad deleted the separate-migration-generator branch Sep 17, 2018
@JakeGinnivan
Copy link
Contributor Author

@JakeGinnivan JakeGinnivan commented Sep 18, 2018

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

@elhigu
Copy link
Member

@elhigu elhigu commented Sep 18, 2018

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

@kibertoad
Copy link
Collaborator

@kibertoad kibertoad commented Sep 18, 2018

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

@elhigu
Copy link
Member

@elhigu 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
Copy link
Collaborator

@kibertoad 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
Copy link
Member

@elhigu 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
Copy link
Collaborator

@kibertoad kibertoad commented Sep 18, 2018

Or that :)

@JakeGinnivan
Copy link
Contributor Author

@JakeGinnivan JakeGinnivan commented Sep 18, 2018

Awesome! Thanks heaps :)

@elhigu
Copy link
Member

@elhigu elhigu commented Sep 18, 2018

And knex@next is now available.

@kibertoad
Copy link
Collaborator

@kibertoad 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
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants