Separate migration generator #2786
Conversation
| import fs from 'fs'; | ||
| import path from 'path'; | ||
| import mkdirp from 'mkdirp'; | ||
| import Promise from 'bluebird'; |
There was a problem hiding this comment.
Can this be replaced with native Promise?
There was a problem hiding this comment.
When I did that, linting failed. Thoughts?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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' }) |
There was a problem hiding this comment.
Can this be rewritten using string template?
src/migrate/Migrator.js
Outdated
| @@ -376,7 +345,7 @@ export default class Migrator { | |||
| ); | |||
| }) | |||
| .then(() => { | |||
| log.push(path.join(directory, name)); | |||
| log.push(name); | |||
There was a problem hiding this comment.
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?
src/migrate/Migrator.js
Outdated
| // are used | ||
| if ( | ||
| config && | ||
| !config.migrationSource && |
There was a problem hiding this comment.
Emm, migration source new functionality leaked into refactoring PR?..
There was a problem hiding this comment.
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
|
Yeah, I will fix this one up now the other PR has been merged. Want me to close for now until I do that? |
|
No need to close, take your time 👍 |
JakeGinnivan
force-pushed
the
separate-migration-generator
branch
2 times, most recently
from
86575a1
to
bf21d10
Compare
|
@elhigu I have updated the PR. the coverage has decreased, not really sure why, thoughts on what tests I should add? |
|
@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. |
|
@JakeGinnivan Please pull changes from tgriesser/knex master, that should resolve the issue |
JakeGinnivan
force-pushed
the
separate-migration-generator
branch
from
bf21d10
to
80a4b53
Compare
|
@kibertoad rebased |
|
Also, if you don't agree with separating these classes, happy for this one to just close. Is just an idea |
|
Personally, I think that this is very much a step in the right direction. |
|
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. |
|
Any chance of a |
|
yep, it is coming I already wrote change logs last night for it. |
|
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. |
|
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. |
|
Or that :) |
|
Awesome! Thanks heaps :) |
|
And knex@next is now available. |
|
There goes my sleep 😂 |
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