Skip to content
This repository has been archived by the owner on Mar 15, 2021. It is now read-only.

Fetch table columns & triggers #14

Merged
merged 9 commits into from
Apr 19, 2016

Conversation

BornaP
Copy link
Member

@BornaP BornaP commented Apr 19, 2016

PR regarding issue #13 .

I implemented API for fetching table columns and triggers related to tables in Sqlectron. This can be consumed in GUI to load table columns on double clicking the table, or for (future) SQL editor autocomplete.

Since it's required in PostgreSQL to define (additional) trigger function to create a trigger, I had to change old db.spec.js a bit.

Code is tested for MySQL and PostgreSQL locally and all the tests are passing. For SQL Server I can not guarantee as I don't have it locally and don't use it regularly, so we will see when it runs on CI. What I noticed regarding SQL Server is that it does not have information_schema for triggers so I used sp_helptrigger.

@BornaP
Copy link
Member Author

BornaP commented Apr 19, 2016

@maxcnunes Seems everything's fine now :)

@@ -119,6 +121,16 @@ async function listRoutines(server, database) {
return database.connection.listRoutines();
}

async function listTableColumns(server, database, table) {
Copy link
Member

Choose a reason for hiding this comment

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

it does not need async

Copy link
Member

Choose a reason for hiding this comment

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

I guess need the async because of the https://github.com/BornaP/sqlectron-core/blob/13-fetch-table-columns/src/db/client.js#L172.
That throws an exception. I guess without the async it will not reject the promise properly.
Since that is the current pattern used for all functions in that file. I will accept without applying your suggestions. We can try out without async in another commit.

@maxcnunes maxcnunes merged commit 82636ef into sqlectron:master Apr 19, 2016
@BornaP BornaP deleted the 13-fetch-table-columns branch April 20, 2016 07:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants