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

CLI cleanup. Do not require client for creating seeds or migrations #2905

Merged
merged 7 commits into from Nov 16, 2018

Conversation

@kibertoad
Copy link
Collaborator

@kibertoad kibertoad commented Nov 13, 2018

Cleanup and improvements after https://github.com/tgriesser/knex/pull/2884/files
fixes #1134

…ient when creating seeds or migrations. cli.js cleanup. Formatting
if (!env.configuration) {
if (opts.client) env.configuration = mkConfigObj(opts);
else
if (!env.pathToKnexFile) {
Copy link
Collaborator Author

@kibertoad kibertoad Nov 13, 2018

I've separated pathToKnexFile and configuration properties to reduce confusion.

exit(
'No knexfile found in this directory. Specify a path with --knexfile'
'No knexfile found in this directory. Specify a path with --knexfile or pass --client and --connection params in commandline'
Copy link
Collaborator Author

@kibertoad kibertoad Nov 13, 2018

I thought this was broken recently, but looks like it was the case for quite a while. Doesn't look like we search for "knexfile in this directory" at all. We probably should.

var ext = (
argv.x ||
env.configuration.ext ||
env.configuration.split('.').pop()
Copy link
Collaborator Author

@kibertoad kibertoad Nov 13, 2018

I have no idea why we were trying to parse knexfile param for extensions, it looks wrong. I'm open to restoring it if someone explains why it was there in the first place.

Copy link
Member

@elhigu elhigu Nov 16, 2018

I suppose it was to automatically use .ts to look migration files in case if extension of knexfile was .ts... or something like that.

I think it was good choice to remove that automation and require people to pass that explicitly. However it is a breaking change, I suppose that whoever wrote that has been using that as a feature.

Copy link
Collaborator Author

@kibertoad kibertoad Nov 16, 2018

But how could it possibly work if env.configuration is either path to knexfile.js or resolved configuration object?

Copy link
Member

@elhigu elhigu Nov 16, 2018

buggily 😛


return wrapper;
}
import { isNumber, isArray, chunk, flatten, assign } from 'lodash';
Copy link
Collaborator Author

@kibertoad kibertoad Nov 13, 2018

No changes in this file, just reformatted with new Prettier version

#!/usr/bin/env jake
'use strict';
/* eslint-disable no-undef */

Copy link
Collaborator Author

@kibertoad kibertoad Nov 13, 2018

added .js extension for proper syntax colouring

function test(description, func) {
const tmpDirPath = os.tmpdir() + '/knex-test-';
let itFails = false;
rimrafSync(tmpDirPath);
Copy link
Collaborator Author

@kibertoad kibertoad Nov 13, 2018

added temp directory deletion before each test

)
));

test('Create a migration file without client passed', (temp) =>
Copy link
Collaborator Author

@kibertoad kibertoad Nov 13, 2018

new test

@kibertoad
Copy link
Collaborator Author

@kibertoad kibertoad commented Nov 13, 2018

@aurium Could you please review?

package.json Outdated
@@ -86,7 +86,7 @@
"plaintest": "mocha --exit -t 10000 -b -R spec test/index.js && npm run tape",
"prepublish": "npm run babel",
"pre_test": "npm run lint",
"bin_test": "jake -f test/jake/*",
"bin_test": "jake -f test/jake/migrate.js",
Copy link
Collaborator Author

@kibertoad kibertoad Nov 13, 2018

at least locally it doesn't execute anything for me unless specific file (or directory with file called "Jakefile") is specified.

Copy link
Member

@elhigu elhigu Nov 16, 2018

Maybe that is platform dependent what it does (shell might expand those wildcards which may break it). I remember having to put wildcard notations in quotes at least with mocha to make globs to work. Like -f "test/jake/*"

Copy link
Collaborator Author

@kibertoad kibertoad Nov 16, 2018

You are probably right, I've checked logs for original PR that introduced CLI tests, and they do indeed show

> jake -f test/jake/*
☑ Create a migration file
☑ Run migrations

So I'll revert this change.

bin/cli.js Outdated
let envName = opts.env || process.env.NODE_ENV || 'development';
let useNullAsDefault = opts.client === 'sqlite3';
const envName = opts.env || process.env.NODE_ENV || 'development';
const useNullAsDefault = opts.client === 'sqlite3';
Copy link
Member

@elhigu elhigu Nov 16, 2018

Did sqlite3 have any aliases for client name. Beacuse of those in tests we always used .driver property from knex.client to check the dialect. I'm not sure how it fits to this case though.

Copy link
Collaborator Author

@kibertoad kibertoad Nov 16, 2018

good point, it does.

});
current = current
.then(() => seed.seed(knex, Promise))
.then(function() {
Copy link
Member

@elhigu elhigu Nov 16, 2018

could be also changed to arrow function

elhigu
elhigu approved these changes Nov 16, 2018
Copy link
Member

@elhigu elhigu left a comment

Looks great 👍

@kibertoad
Copy link
Collaborator Author

@kibertoad kibertoad commented Nov 16, 2018

@elhigu Thanks! It was an insanely bad day of a pretty bad month, really appreciate something positive :D

@kibertoad kibertoad merged commit 887fb53 into master Nov 16, 2018
3 checks passed
@kibertoad kibertoad deleted the general/cli-cleanup branch Nov 16, 2018
mwilliammyers added a commit to voxjar/knex that referenced this issue Dec 12, 2018
…nex#2905)

* Specify jakefile explicitly to ensure it being run. Do not require client when creating seeds or migrations. cli.js cleanup. Formatting

* Fix message

* Fix typo

* Ignore console rule in CLI tests

* Fix rimraf import

* Improvements after code review

* One more arrow function
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.

2 participants