-
Notifications
You must be signed in to change notification settings - Fork 3
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
fix: type generation on load #648
Conversation
return isPrimaryKey || isNullable ? false : true; | ||
} | ||
|
||
getRequiredStatus(columnDef, nullable) { | ||
const hasDefaultValue = | ||
columnDef.hasOwnProperty("default_val") && columnDef.default_val != null; | ||
Object.prototype.hasOwnProperty.call(columnDef, "default_val") && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Modified all access methods to avoid shadowing
} | ||
} | ||
} | ||
dbSchema[tableName] = columns; | ||
} else if (statement.type === "alter") { | ||
// Process ALTER TABLE statements | ||
let newConstraint = {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the point in moving the contraint variable outside the for loop? It's scope is to apply to the alterSpec in question.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was just due to a stricter eslint configuration where there was a lexical declaration inside a case block. To fix the issue, I moved the lexical declaration outside of the case block.
…s-are-not-generated-onload
Sorry, let this sit and forgot about rerequesting are review here. Are we good to merge here? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Just one callout to verify something before merging.
@@ -141,11 +142,11 @@ export class PgSchemaTypeGen { | |||
} | |||
|
|||
addColumn(columnDef, columns) { | |||
const columnName = columnDef.column.column.expr.value; | |||
const columnName = columnDef.column.column; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this correct? I thought the location of the column name was changed after the update? Can you verify this is the right way to get the name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep! This should be it! I verified locally with the latest version we have of node-sql-parser and the object we from astify.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
confirmed by pulling and it works for me
The object post astify() we receive from node-sql-parser changed. We can think about Version Control in the future.
Quick fix here: returning the object to how it was and adding an additional layer on Editor to ensure mounting of types