From <[email protected]>: [email protected] 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) Thanks for the comments Ian. Resolved most of them. There are two that I'll have to look into a little more. 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? Not sure, I think it is something to do with updating the Ngx modules. I did not add this manually. 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 Done 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 Done 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 Done 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? Done 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 Done 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 Done 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. […] Done 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 […] Done 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? Done 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 Done 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 Done 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 ta […] Done 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? it was for displaying the pure JSON result returned. Fixed it (it should be the first thing added to data). 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 Done 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 Done 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 Done 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 Done 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? not quite sure...i remember it giving me some issues at the very start to build, so I took it out. I'll look into it more. 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 Done -- 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-Reviewer: [email protected] Gerrit-CC: Till Westmann <[email protected]> Gerrit-Comment-Date: Fri, 30 Apr 2021 19:36:46 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Ian Maxon <[email protected]> Gerrit-MessageType: comment
