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

added support for postgres enums #17

Closed
wants to merge 1 commit into from

Conversation

Zhurik
Copy link

@Zhurik Zhurik commented Aug 3, 2022

No description provided.

Copy link
Owner

@KarnerTh KarnerTh left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! And sorry for the late response, I was quite busy lately.

The changes LGTM, but the test would need to be fixed. If you are motivated you could also add new tests that cover the new functionality.

If you do not have the time to make the changes please let me know, I will happily merge back the current state and finish it up by myself.

Todos:

  • fix the test
  • add new tests for enum columns
  • add a flag to enable/disable the new functionality (I would suggest to default to disabled, so that the diagram does not get polluted due to large enums)
  • update readme with the new flag

@@ -114,7 +114,7 @@ func TestDatabaseIntegrations(t *testing.T) {
}

assert.Nil(t, err)
assert.ElementsMatch(t, testCase.expectedColumns, columnNames)
//assert.ElementsMatch(t, testCase.expectedColumns, columnNames)
Copy link
Owner

Choose a reason for hiding this comment

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

This would need to be fixed

@@ -78,8 +78,9 @@ func (c mySqlConnector) GetTables(schemaName string) ([]string, error) {
}

func (c mySqlConnector) GetColumns(tableName string) ([]ColumnResult, error) {
// FIXME add support for MySQL enums
Copy link
Owner

Choose a reason for hiding this comment

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

If you are unfamiliar with MySQL I would fix this once it was merged back :)

@KarnerTh KarnerTh mentioned this pull request Nov 30, 2022
@KarnerTh
Copy link
Owner

Get's closed in favour for #26 because there is no response from the contributor

@KarnerTh KarnerTh closed this Nov 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants