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

Projects
None yet
2 participants
@kibertoad
Copy link
Collaborator

kibertoad commented Nov 13, 2018

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

Specify jakefile explicitly to ensure it being run. Do not require cl…
…ient when creating seeds or migrations. cli.js cleanup. Formatting
if (!env.configuration) {
if (opts.client) env.configuration = mkConfigObj(opts);
else
if (!env.pathToKnexFile) {

This comment has been minimized.

@kibertoad

kibertoad Nov 13, 2018

Collaborator

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'

This comment has been minimized.

@kibertoad

kibertoad Nov 13, 2018

Collaborator

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()

This comment has been minimized.

@kibertoad

kibertoad Nov 13, 2018

Collaborator

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.

This comment has been minimized.

@elhigu

elhigu Nov 16, 2018

Collaborator

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.

This comment has been minimized.

@kibertoad

kibertoad Nov 16, 2018

Collaborator

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

This comment has been minimized.

@elhigu

elhigu Nov 16, 2018

Collaborator

buggily 😛


return wrapper;
}
import { isNumber, isArray, chunk, flatten, assign } from 'lodash';

This comment has been minimized.

@kibertoad

kibertoad Nov 13, 2018

Collaborator

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

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

This comment has been minimized.

@kibertoad

kibertoad Nov 13, 2018

Collaborator

added .js extension for proper syntax colouring

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

This comment has been minimized.

@kibertoad

kibertoad Nov 13, 2018

Collaborator

added temp directory deletion before each test

)
));

test('Create a migration file without client passed', (temp) =>

This comment has been minimized.

@kibertoad

kibertoad Nov 13, 2018

Collaborator

new test

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Nov 13, 2018

@aurium Could you please review?

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

This comment has been minimized.

@kibertoad

kibertoad Nov 13, 2018

Collaborator

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

This comment has been minimized.

@elhigu

elhigu Nov 16, 2018

Collaborator

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/*"

This comment has been minimized.

@kibertoad

kibertoad Nov 16, 2018

Collaborator

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.

kibertoad added some commits Nov 13, 2018

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';

This comment has been minimized.

@elhigu

elhigu Nov 16, 2018

Collaborator

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.

This comment has been minimized.

@kibertoad

kibertoad Nov 16, 2018

Collaborator

good point, it does.

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

This comment has been minimized.

@elhigu

elhigu Nov 16, 2018

Collaborator

could be also changed to arrow function

@elhigu

elhigu approved these changes Nov 16, 2018

Copy link
Collaborator

elhigu left a comment

Looks great 👍

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Nov 16, 2018

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

kibertoad added some commits Nov 16, 2018

@kibertoad kibertoad merged commit 887fb53 into master Nov 16, 2018

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.02%) to 84.788%
Details

@kibertoad kibertoad deleted the general/cli-cleanup branch Nov 16, 2018

mwilliammyers added a commit to voxjar/knex that referenced this pull request Dec 12, 2018

CLI cleanup. Do not require client for creating seeds or migrations (t…
…griesser#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