Re: [Feature-3452] Schema diff tool with Table, View, Materialized View, Functions and Procedures
От | Aditya Toshniwal |
---|---|
Тема | Re: [Feature-3452] Schema diff tool with Table, View, Materialized View, Functions and Procedures |
Дата | |
Msg-id | CAM9w-_ms63K4cc1zFoyq1hfuoe==KeNmAg2_e4=QAdFzUNjpZQ@mail.gmail.com обсуждение исходный текст |
Ответ на | [Feature-3452] Schema diff tool with Table, View, Materialized View,Functions and Procedures (Akshay Joshi <akshay.joshi@enterprisedb.com>) |
Список | pgadmin-hackers |
Hi Akshay/Khushboo,
I have few suggestions/questions for the attached patch:
- Code like SchemaDiffRegistry('server', ServerNode) should be replaced with SSchemaDiffRegistry(ServerModule.NODE_TYPE, ServerNode)
- The variables return_ajax_response, only_sql, json_resp as far as I understood are similar. Can we have same var name everywhere ?
- Remove commented code - web/pgadmin/browser/server_groups/servers/databases/schemas/tables/__init__.py -> get_sql_from_table_diff
- In web/pgadmin/browser/server_groups/servers/databases/schemas/tables/__init__.py -> fetch_tables - keys_to_remove is passed. How is it different from keys_to_ignore used at other places ?
- web/pgadmin/tools/schema_diff/__init__.py -> check_version_compatibility has hardcoded version numbers. Can we use get_version_mapping_directories from web/pgadmin/utils/versioned_template_loader.py ?
- Rename .reallyHidden to .really-hidden
- CSS class #schema-diff-grid -> background: white; - hardcoded color can be changed to use $color-bg instead. Also use rem or px for - font-size: 9pt.
- .slick-group-toggle.collapsed, .slick-group-toggle.expanded - svgs are not required. Font awesome has the icons. refer - .obj_properties .collapsed .caret::before.
- In web/pgadmin/tools/schema_diff/static/js/schema_diff.backform.js -> formatNode - Appends can be avoided and formed in a single statement.
+ } else {
+ return $('<span></span>').append(
+ $('<span></span>', {
+ class: 'wcTabIcon ' + optimage,
+ })
+ ).append($('<span></span>').text(opt.text));
+ }
+}; - In web/pgadmin/tools/schema_diff/static/js/schema_diff.backform.js -> fetchData - We should not use async = false.+ $.ajax({
+ async: false,
+ url: url,
+ }) - In web/pgadmin/tools/schema_diff/static/js/schema_diff_ui.js - Use 'sources/window' - pgWindow.
+ let preferences = (window.opener !== null) ? window.opener.pgAdmin.Browser.get_preferences_for_module('schema_diff') : window.top.pgAdmin.Browser.get_preferences_for_module('schema_diff'); - In web/pgadmin/tools/schema_diff/static/js/schema_diff_ui.js - Use map instead of for loop. It will also remove the sel_rows_data.push. will be helpfull in large data.
+ for (var row = 0; row < sel_rows.length; row++) {
+ let data = self.grid.getData().getItem(sel_rows[row]);
+
+ if (data.type) {
+ let tmp_data = {
+ 'node_type': data.type,
+ 'source_oid': parseInt(data.oid, 10),
+ 'target_oid': parseInt(data.oid, 10),
+ 'comp_status': data.status,
+ };
+
+ if(data.status && (data.status.toLowerCase() == 'different' || data.status.toLowerCase() == 'identical')) {
+ tmp_data['target_oid'] = data.target_oid;
+ }
+ sel_rows_data.push(tmp_data);
+ }
+ }
+
+ url_params['sel_rows'] = sel_rows_data; - In web/pgadmin/tools/schema_diff/static/js/schema_diff_ui.js -Are we doing anything to handle failure.
+ connect_database(server_id, db_id, callback) {
+ var url = url_for('schema_diff.connect_database', {'sid': server_id, 'did': db_id});
+ $.post(url)
+ .done(function(res) {
+ if (res.success && res.data) {
+ callback(res.data);
+ }
+ })
+ .fail(function() {
+ // Fail
+ });
+
+ } - As you've added a completely different function for connect_server, I would suggest to rename dlgServerPass to a different name to avoid conflict with existing dlgServerPass in server.js
+ if (!Alertify.dlgServerPass) {
+ Alertify.dialog('dlgServerPass', function factory() { - Generate script does not work if pgAdmin opened in iframe. Iframes are used by tools like Katacoda.
- Comparing objects loader is not attached to DDL Comparison panel.
- Filter icon and Generate script icon size are different. Also change icons CSS to use font-icon. You can refer icons from sqleditor.
*The fetch_objects_to_compare function used in each node uses loop to fetch data. Although it is working for now, but I would suggest using bulk fetch nodes instead of looping through all the nodes one by one.*
On Fri, Dec 27, 2019 at 4:55 PM Aditya Toshniwal <aditya.toshniwal@enterprisedb.com> wrote:
Hi Akshay/Khushboo,I have few suggestions/questions for the attached patch:
- Code like SchemaDiffRegistry('server', ServerNode) should be replaced with SSchemaDiffRegistry(ServerModule.NODE_TYPE, ServerNode)
- The variables return_ajax_response, only_sql, json_resp as far as I understood are similar. Can we have same var name everywhere ?
- Remove commented code - web/pgadmin/browser/server_groups/servers/databases/schemas/tables/__init__.py -> get_sql_from_table_diff
- In web/pgadmin/browser/server_groups/servers/databases/schemas/tables/__init__.py -> fetch_tables - keys_to_remove is passed. How is it different from keys_to_ignore used at other places ?
- web/pgadmin/tools/schema_diff/__init__.py -> check_version_compatibility has hardcoded version numbers. Can we use get_version_mapping_directories from web/pgadmin/utils/versioned_template_loader.py ?
- Rename .reallyHidden to .really-hidden
- CSS class #schema-diff-grid -> background: white; - hardcoded color can be changed to use $color-bg instead. Also use rem or px for - font-size: 9pt.
- .slick-group-toggle.collapsed, .slick-group-toggle.expanded - svgs are not required. Font awesome has the icons. refer - .obj_properties .collapsed .caret::before.
- In web/pgadmin/tools/schema_diff/static/js/schema_diff.backform.js -> formatNode - Appends can be avoided and formed in a single statement.
+ } else {
+ return $('<span></span>').append(
+ $('<span></span>', {
+ class: 'wcTabIcon ' + optimage,
+ })
+ ).append($('<span></span>').text(opt.text));
+ }
+};- In web/pgadmin/tools/schema_diff/static/js/schema_diff.backform.js -> fetchData - We should not use async = false.
+ $.ajax({
+ async: false,
+ url: url,
+ })- In web/pgadmin/tools/schema_diff/static/js/schema_diff_ui.js - Use 'sources/window' - pgWindow.
+ let preferences = (window.opener !== null) ? window.opener.pgAdmin.Browser.get_preferences_for_module('schema_diff') : window.top.pgAdmin.Browser.get_preferences_for_module('schema_diff');- In web/pgadmin/tools/schema_diff/static/js/schema_diff_ui.js - Use map instead of for loop. It will also remove the sel_rows_data.push. will be helpfull in large data.
+ for (var row = 0; row < sel_rows.length; row++) {
+ let data = self.grid.getData().getItem(sel_rows[row]);
+
+ if (data.type) {
+ let tmp_data = {
+ 'node_type': data.type,
+ 'source_oid': parseInt(data.oid, 10),
+ 'target_oid': parseInt(data.oid, 10),
+ 'comp_status': data.status,
+ };
+
+ if(data.status && (data.status.toLowerCase() == 'different' || data.status.toLowerCase() == 'identical')) {
+ tmp_data['target_oid'] = data.target_oid;
+ }
+ sel_rows_data.push(tmp_data);
+ }
+ }
+
+ url_params['sel_rows'] = sel_rows_data;- In web/pgadmin/tools/schema_diff/static/js/schema_diff_ui.js -Are we doing anything to handle failure.
+ connect_database(server_id, db_id, callback) {
+ var url = url_for('schema_diff.connect_database', {'sid': server_id, 'did': db_id});
+ $.post(url)
+ .done(function(res) {
+ if (res.success && res.data) {
+ callback(res.data);
+ }
+ })
+ .fail(function() {
+ // Fail
+ });
+
+ }- As you've added a completely different function for connect_server, I would suggest to rename dlgServerPass to a different name to avoid conflict with existing dlgServerPass in server.js
+ if (!Alertify.dlgServerPass) {
+ Alertify.dialog('dlgServerPass', function factory() {- Generate script does not work if pgAdmin opened in iframe. Iframes are used by tools like Katacoda.
- Comparing objects loader is not attached to DDL Comparison panel.
- Filter icon and Generate script icon size are different. Also change icons CSS to use font-icon. You can refer icons from sqleditor.
*The fetch_objects_to_compare function used in each node uses loop to fetch data. Although it is working for now, but I would suggest using bulk fetch nodes instead of looping through all the nodes one by one.*On Fri, Dec 20, 2019 at 6:59 PM Akshay Joshi <akshay.joshi@enterprisedb.com> wrote:Hi Hackers,Attached is the implementation of the new feature Schema Diff Tool. Initial work(backend code to compare the objects) has been done by me and then most of the task has been completed by Khushboo Vashi. Sending the patch on behalf of her.Currently, this tool only supports Tables, Views, Materialized Views, Functions and Procedures node.Please review and test it thoroughly. Suggestions are welcome to improve the tool.--Thanks & RegardsAkshay JoshiSr. Software ArchitectEnterpriseDB Software India Private LimitedMobile: +91 976-788-8246--Thanks and Regards,Aditya ToshniwalpgAdmin Hacker | Sr. Software Engineer | EnterpriseDB India | Pune"Don't Complain about Heat, Plant a TREE"
Thanks and Regards,
Aditya Toshniwal
pgAdmin Hacker | Sr. Software Engineer | EnterpriseDB India | Pune
"Don't Complain about Heat, Plant a TREE"
Вложения
В списке pgadmin-hackers по дате отправления:
Предыдущее
От: Nagesh DhopeДата:
Сообщение: Re: [pgAdmin4][RM#4772] Add aria-label attribute to buttons used ingraphical explain plan
Следующее
От: Akshay JoshiДата:
Сообщение: pgAdmin 4 commit: Refactored SQL of Functions and Procedures. Fixes #50