From Ian Maxon <[email protected]>: Ian Maxon has posted comments on this change. ( https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/9152 )
Change subject: [NO ISSUE][DASHBOARD] New Asterix Query Console ...................................................................... Patch Set 30: (20 comments) 1st pass https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/9152/30/asterixdb/asterix-dashboard/src/node/src/app/app.module.ts File asterixdb/asterix-dashboard/src/node/src/app/app.module.ts: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/9152/30/asterixdb/asterix-dashboard/src/node/src/app/app.module.ts@69 PS30, Line 69: runtimeChecks: { : strictStateImmutability: false, : strictActionImmutability: false, : strictStateSerializability: false, : strictActionSerializability: false, : }, whats this about? https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/9152/30/asterixdb/asterix-dashboard/src/node/src/app/dashboard/query/input.component.ts File asterixdb/asterix-dashboard/src/node/src/app/dashboard/query/input.component.ts: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/9152/30/asterixdb/asterix-dashboard/src/node/src/app/dashboard/query/input.component.ts@304 PS30, Line 304: : //let output component know about error : : /* : let error_line_regex = /(in line )(\d+)/i : let error_col_regex = /(at column )(\d+)/i : let error_line = 0; : let error_col = 0; : : if (error_line_regex.test(this.queryErrorMessageString) && error_col_regex.test(this.queryErrorMessageString)) { : error_line = parseInt(this.queryErrorMessageString.match(error_line_regex)[2]); : error_col = parseInt(this.queryErrorMessageString.match(error_col_regex)[2]); : } : : if (error_line > 0 && error_col > 0) { : //this.editor.markText({line: error_line, ch: error_col}, {line: error_line, ch: error_col + 1}); : this.editor.markText({line: 1, ch:1}, {line:1, ch: 15}); : this.editor.focus(); : } : */ lets get rid of the commented code unless it's a frequently toggled debug or something https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/9152/30/asterixdb/asterix-dashboard/src/node/src/app/dashboard/query/input.component.ts@341 PS30, Line 341: //let use_regex = /use.*?;(?=\n)/i same here https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/9152/30/asterixdb/asterix-dashboard/src/node/src/app/dashboard/query/input.component.ts@378 PS30, Line 378: //this.history.push(this.queryString); same here https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/9152/30/asterixdb/asterix-dashboard/src/node/src/app/dashboard/query/input.component.ts@555 PS30, Line 555: //this.editor = CodeMirror.fromTextArea(this.editor.nativeElement, config); : this.editor = CodeMirror.fromTextArea(this.editor.nativeElement, { : mode: { : name: "sql", : keywords: this.set(this.sqlppKeywords), : builtin: this.set(this.sqlppTypes), : atoms: this.set("true false null missing"), : dateSQL: this.set("date time datetime duration year_month_duration day_time_duration interval"), : support: this.set("ODBCdotTable doubleQuote binaryNumber hexNumber commentSlashSlash") : }, : lineWrapping: true, : showCursorWhenSelecting: true, : autofocus: true, : lineNumbers: true, : autoCloseBrackets: { : pairs: "()[]{}''\"\"``", : closeBefore: ")]}'\":;>", : triples: "", : explode: "[]{}()", : override: true : }, : extraKeys: {"Ctrl-Space": "autocomplete"}, : hint: CodeMirror.hint.sql, : }); : this.editor.setSize(null, 'auto'); : this.editor.getDoc().setValue(this.queryString); : this.editor.on('changes', () => { : this.queryString = this.editor.getValue(); : }); isn't a lot of this duplicated earlier in this file? https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/9152/30/asterixdb/asterix-dashboard/src/node/src/app/dashboard/query/metadata.component.ts File asterixdb/asterix-dashboard/src/node/src/app/dashboard/query/metadata.component.ts: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/9152/30/asterixdb/asterix-dashboard/src/node/src/app/dashboard/query/metadata.component.ts@195 PS30, Line 195: to_create camel case https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/9152/30/asterixdb/asterix-dashboard/src/node/src/app/dashboard/query/metadata.component.ts@207 PS30, Line 207: list_type camel case https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/9152/30/asterixdb/asterix-dashboard/src/node/src/app/dashboard/query/metadata.component.ts@208 PS30, Line 208: : if (to_create.DatatypeType == "RECORD") { : to_create.DatatypeType = "Record"; : } : if (to_create.DatatypeType == "ORDEREDLIST") { : to_create.DatatypeType = "Ordered List"; : list_type = "OrderedList"; : } : if (to_create.DatatypeType == "UNORDEREDLIST") { : to_create.DatatypeType = "Unordered List"; : list_type = "UnorderedList"; : } can this be a switch/case with the string constants here in an enum or something. that way if we ever change this it will live in one place https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/9152/30/asterixdb/asterix-dashboard/src/node/src/app/dashboard/query/metadata.component.ts@227 PS30, Line 227: if ((data.DataverseName + "." + field.FieldType) in this.datatypesDict && this.datatypes[this.datatypesDict[data.DataverseName + "." + field.FieldType]].Derived != undefined) { is there an auto-formatter enabled for this stuff? should be split here after && onto a new line or something https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/9152/30/asterixdb/asterix-dashboard/src/node/src/app/dashboard/query/metadata.component.ts@231 PS30, Line 231: this.datatypesDict[data.DataverseName + "." + field.FieldType make this a variable b/c it's used here and in the condition and it's big? https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/9152/30/asterixdb/asterix-dashboard/src/node/src/app/dashboard/query/metadata.component.ts@250 PS30, Line 250: recurse_result camel case https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/9152/30/asterixdb/asterix-dashboard/src/node/src/app/dashboard/query/metadata.component.ts@254 PS30, Line 254: to_add camel case https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/9152/30/asterixdb/asterix-dashboard/src/node/src/app/dashboard/query/metadata.component.ts@263 PS30, Line 263: let nested_list_type = ""; : let nested_list_type_name = ""; these should be camel cased or better yet somehow part of an enum or class that lets you slap the tag in and get these out https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/9152/30/asterixdb/asterix-dashboard/src/node/src/app/dashboard/query/metadata.component.ts@389 PS30, Line 389: data.guts = metadata; this is for debug right? should it stay or be hidden or sth? https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/9152/30/asterixdb/asterix-dashboard/src/node/src/app/dashboard/query/plan-viewer.component.ts File asterixdb/asterix-dashboard/src/node/src/app/dashboard/query/plan-viewer.component.ts: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/9152/30/asterixdb/asterix-dashboard/src/node/src/app/dashboard/query/plan-viewer.component.ts@208 PS30, Line 208: ode_to_add camel case https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/9152/30/asterixdb/asterix-dashboard/src/node/src/app/dashboard/query/plan-viewer.component.ts@271 PS30, Line 271: node_drop_down camel case https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/9152/30/asterixdb/asterix-dashboard/src/node/src/app/shared/effects/cancel.effects.ts File asterixdb/asterix-dashboard/src/node/src/app/shared/effects/cancel.effects.ts: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/9152/30/asterixdb/asterix-dashboard/src/node/src/app/shared/effects/cancel.effects.ts@31 PS30, Line 31: the remove https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/9152/30/asterixdb/asterix-dashboard/src/node/src/app/shared/effects/dataset.effects.ts File asterixdb/asterix-dashboard/src/node/src/app/shared/effects/dataset.effects.ts: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/9152/30/asterixdb/asterix-dashboard/src/node/src/app/shared/effects/dataset.effects.ts@22 PS30, Line 22: Action_type PascalCase https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/9152/30/asterixdb/asterix-dashboard/src/node/src/polyfills.ts File asterixdb/asterix-dashboard/src/node/src/polyfills.ts: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/9152/30/asterixdb/asterix-dashboard/src/node/src/polyfills.ts@46 PS30, Line 46: //import 'core-js/es7/reflect'; 'sup with this? https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/9152/30/asterixdb/asterix-dashboard/src/node/src/styles/_general.scss File asterixdb/asterix-dashboard/src/node/src/styles/_general.scss: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/9152/30/asterixdb/asterix-dashboard/src/node/src/styles/_general.scss@7 PS30, Line 7: se remove -- To view, visit https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/9152 To unsubscribe, or for help writing mail filters, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Change-Id: I6d8ee6b1bfb1a6e5dc7072566d76841f9123b093 Gerrit-Change-Number: 9152 Gerrit-PatchSet: 30 Gerrit-Owner: [email protected] Gerrit-Reviewer: Anon. E. Moose #1000171 Gerrit-Reviewer: Ian Maxon <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-CC: Till Westmann <[email protected]> Gerrit-Comment-Date: Tue, 27 Apr 2021 02:31:29 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
