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

#125 objects sql definitions #141

Merged
merged 12 commits into from
May 12, 2016

Conversation

BornaP
Copy link
Member

@BornaP BornaP commented May 10, 2016

PR resolving (partially - not yet for routines) issue #125 .

So here it is in short:

  • Database objects are now moved into database-item.jsx component because of context menu. I found this a cleanest solution, because context menu should be a property of database object (all links in menu are related exclusively to that object).
  • Context menu is built dynamically here .Reason, as explained in comment in a code, is that there's no need to built context menu for each object (table/view) before it's needed, it just slows down application, especially when loading big number of objects (test on db with 130 tables, it took 2 secs). This way is much faster and neater.
  • @maxcnunes Here I need your opinion, whether to keep fetching scripts (SQL definitions) in separate functions (like its now, every scripts has it's own implementation in actions/sqlscripts.js) or make unique one (i.e. getScriptIfNeeded) and pass script type (I guess this would be a object, i.e. { type: 'create', item: 'table' } ) as a parameter to single function (something like this getScriptIfNeeded (database, table, scriptNeeded) ). Then we would have if-else clause which would call on a database (sqlectron-core) API, depending on the parameter passed. I know this is probably anti-pattern, but it would be much less code than it is now, and only one event handler function would be passed to components (now we have like 7).
  • Context menu for routines not implemented yet. Should be fairly simple, because they are already fetched in store routines object.

@BornaP
Copy link
Member Author

BornaP commented May 10, 2016

Oh and by the way, execute default query is moved to context menu. This is because a single table click fetches columns/triggers, so double clicking on it was not a good UX.

const scriptsNames = ['CREATE', 'SELECT', 'INSERT', 'UPDATE', 'DELETE'];

this.contextMenu = new Menu();
if (dbObjectType === 'Tables' || dbObjectType === 'Views') {
Copy link
Member

Choose a reason for hiding this comment

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

Could you avoid nested if and else statements. For instance this first if could be changed to leave the function. And the next if leave the function after appending the menu item.

Copy link
Member Author

@BornaP BornaP May 10, 2016

Choose a reason for hiding this comment

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

Sure, will do!

@maxcnunes
Copy link
Member

Since all sqlscripts has the same behavior I guess is good reason to gather them all.
By the way awesome PR 👍

@BornaP
Copy link
Member Author

BornaP commented May 10, 2016

Alright then, I'll try to finish this tommorow :)

@BornaP
Copy link
Member Author

BornaP commented May 11, 2016

Refactored code, added fetching functions/procedures SQL definitions and fixed behavior so it updates SQL editor when user selects script he selected earlier (so it's already fetched on front-end).

There are two thing that should be changed in future, but I think they should be separate PR's:

  • Handling function/procedure SQL definition: This should have an API call on sqlectron-core, instead of current logic. Current logic is kind of a hack, we just read current store object and dispatch updateQueryIfNeeded action in query-browser container. I think it's better to make changes on back-end, so this has it's own API call, like fetching table/view scripts. Beside having a cleaner code on front-end, we could enhance SQL definition of routines for MySQL/PostgreSQL that way (put CREATE statement around it etc.). I was thinking about implementing this next, but will do that on another PR since it needs changes on back-end too.
  • Append script to current query in editor - Queries actions should be extended with appendQueryIfNeeded. I think it's better to handle previewing scripts that way than updating whole query (like it's now) or opening new tab. Reason - in most use cases when you use default scripts you already have some kind of query or you want to use more scripts at the same time. Simple use-case, mostly when I use create/update/delete scripts I use select script to see changes made. Anyways, it's not a big issue, so I guess this too can go into a another PR if we agree on implementing this.

Well, I guess this is it for this PR. @maxcnunes @krolow any thoughts? :)

click: onExecuteDefaultQuery.bind(this, database, item)
}));
this.contextMenu.append(new MenuItem({
label: `CREATE script`,
Copy link
Member

Choose a reason for hiding this comment

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

does not need to use template string

@maxcnunes
Copy link
Member

Related to your 2 suggestions. Please do that 👍
Feel free to merge it into master after you have updated the sqlectron-core version.

@BornaP
Copy link
Member Author

BornaP commented May 12, 2016

With last commit upgraded to newest sqlectron-core and changed the way of fetching routines SQL (create statement) definition. Also, removed those unnecessary template strings as @krolow suggested.

Regarding second suggestion. Since adding appendQueryIfNeeded to queries actions is self-contained change, I'll implement that in separate PR.

@BornaP
Copy link
Member Author

BornaP commented May 12, 2016

Since Travis CI build failure is related to another change and not this PR, I'll merge it into master.

@BornaP BornaP merged commit d79069f into sqlectron:master May 12, 2016
@BornaP BornaP deleted the 125-objects-sql-definitions branch May 14, 2016 09:31
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.

3 participants