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.

Sign up for GitHub

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

Jump to bottom

cli for schema generator should use orm config more #2912

Closed
wants to merge 2 commits into from
Closed

cli for schema generator should use orm config more #2912

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e90ad6f
Do not set wrap params key / value when fkChecks arg not present
m11m Mar 14, 2022
14ed281
Merge branch 'mikro-orm:master' into master
m11m Mar 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion packages/cli/src/commands/SchemaCommandFactory.ts
View file
Open in desktop
Expand Up @@ -97,7 +97,8 @@ export class SchemaCommandFactory {

const orm = await CLIHelper.getORM() as MikroORM<AbstractSqlDriver>;
const generator = new SchemaGenerator(orm.em);
const params = { wrap: !args.fkChecks, ...args };
let params = args.fkChecks !== undefined ? {wrap: !args.fkChecks} : {};
params = {...params, ...args};
Comment on lines +100 to +101
Copy link
Member

@B4nan B4nan Mar 15, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i dont understand how this can fix anything, it should do exactly the same thing is the previous version, but more verbose/less readable.

more importantly, if there is a bug, we need a test case

edit: ok i get it, if there is a value, we ignore the global config. i should have renamed the wrap thing, its confusing on its own...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I apologize for not knowing about the rules, such as needing a test case. I don't know much about this project and was only trying to learn a little about it when I found the issue.


if (args.dump) {
const m = `get${method.substr(0, 1).toUpperCase()}${method.substr(1)}SchemaSQL` as 'getCreateSchemaSQL' | 'getUpdateSchemaSQL' | 'getDropSchemaSQL';
Expand Down