stevedlawrence commented on code in PR #1177: URL: https://github.com/apache/daffodil-vscode/pull/1177#discussion_r1979896273
########## src/rootCompletion/utils.ts: ########## @@ -0,0 +1,106 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import * as fs from 'fs' +import * as vscode from 'vscode' +import { queryXML } from 'xmlUtils' + +// Method to create simple completion item given provided value and position +export function getSimpleCompletionItem( + value: string, + position: vscode.Position +): vscode.CompletionItem { + let item = new vscode.CompletionItem( + `${value}`, + vscode.CompletionItemKind.Value + ) + item.range = new vscode.Range(position, position) + return item +} + +// Method to get previous lines based on i. +function getPrevLines( + document: vscode.TextDocument, + i: number +): vscode.TextLine[] { + let prevLines: vscode.TextLine[] = [] + + for (var j = 0; j < 4; j++) { + if (i > j) { + prevLines.push(document.lineAt(i - (j + 1))) + } + } + + return prevLines +} + +// Method to get the schema path from the configuration file being edited +export function getSchemaPathFromLaunchJSON( Review Comment: Is there a reason this function is implemented the way it is? It looks like it looks at the previous four lines for "path", then if it finds that it looks at the previous for lines of that for "schema", and then does some str replace to get the path? This seems fragile and the 4 constant seems a bit arbitrary. For example, what if the `path` or `schema` is more than four behind, or is after the current line? Why not not scan the entire json file? These are relatively small files so I imagine it woudl be pretty fast. Or alternatively, what about loading the json into a variable and accessing the `schema/path` members directly? I guess that breaks if the json isn't valid, but is that a concern? Or is this just how completion things are supposed to be written and are only supposed to look at immediate lines for context? ########## src/rootCompletion/rootNamespace.ts: ########## @@ -0,0 +1,70 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import * as vscode from 'vscode' +import { getSimpleCompletionItem, getPossibleItems } from './utils' + +export function checkForRootNamespaceItems( + document: vscode.TextDocument, + position: vscode.Position +): vscode.CompletionItem[] { + if ( + !document.fileName.includes('launch.json') || + !document.lineAt(position).text.includes('"rootNamespace"') + ) + return [] + + const possibleRootNamespaces = getPossibleItems( + document, + position, + 'xs:schema', Review Comment: Same comment here as a above, `xs` isn't necessarily the right prefix. ########## src/rootCompletion/utils.ts: ########## @@ -0,0 +1,106 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import * as fs from 'fs' +import * as vscode from 'vscode' +import { queryXML } from 'xmlUtils' + +// Method to create simple completion item given provided value and position +export function getSimpleCompletionItem( + value: string, + position: vscode.Position +): vscode.CompletionItem { + let item = new vscode.CompletionItem( + `${value}`, + vscode.CompletionItemKind.Value + ) + item.range = new vscode.Range(position, position) + return item +} + +// Method to get previous lines based on i. +function getPrevLines( + document: vscode.TextDocument, + i: number +): vscode.TextLine[] { + let prevLines: vscode.TextLine[] = [] + + for (var j = 0; j < 4; j++) { + if (i > j) { + prevLines.push(document.lineAt(i - (j + 1))) + } + } + + return prevLines +} + +// Method to get the schema path from the configuration file being edited +export function getSchemaPathFromLaunchJSON( + document: vscode.TextDocument, + position: vscode.Position +): string { + let schemaPath = '' + + for (var i = position.line - 4; i < position.line; i++) { + if (i <= 0) continue + + let currLine = document.lineAt(i) + if (!currLine.text.includes('path')) continue + + const prevLines = getPrevLines(document, i) + + let isUnderSchema = + prevLines + .map((prevLines) => prevLines.text.includes('schema')) + .filter((b) => b == true).length > 0 + + if (isUnderSchema) { + schemaPath = currLine.text + .replaceAll(' ', '') + .replaceAll('"', '') + .replaceAll(',', '') + .replace('path:', '') Review Comment: This also feels a bit fragile. For example, if a path has a space in it the space will just be removed. Or if there are multiple values on the same line separated by a comma, things will just get concatenated together? What about using regex to extract the value? Something like this maybe: ```regex (["'])path\1\s*:\s*(["'])(.*?)\2 ``` Capture group 3 will be the value of path. It's a bit ugly, but avoids possible edge cases. ########## src/rootCompletion/rootName.ts: ########## @@ -0,0 +1,68 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import * as vscode from 'vscode' +import { getSimpleCompletionItem, getPossibleItems } from './utils' + +export function checkForRootNameItems( + document: vscode.TextDocument, + position: vscode.Position +): vscode.CompletionItem[] { + if ( + !document.fileName.includes('launch.json') || + !document.lineAt(position).text.includes('"rootName"') + ) + return [] + + const possibleRootNames: string[] = getPossibleItems( + document, + position, + 'xs:element', Review Comment: Looking for elements with the xs prefix is potential fragile. We sometimes see "xsd" as an alternative prefix, and more and more schemas are written so the "xs:" prefix isn't needed at all (e.g. `<element name="foo">...`. I would suggest either passing in a namespace instead of a prefix and then look for elements that have a prefix that maps to that namespace, or maybe more simply just ignore namespaces/prefixes and assume the schema document uses correct namespaces/prefixes. It looks like the way fast-xml-parser it makes dealing with namespaces/prefixes diffciult, I would suggest doing the latter and setting the fast-xml-parser option `removeNSPrefix` to true and not worry about them. ########## yarn.lock: ########## @@ -4280,6 +4287,11 @@ strip-json-comments@~2.0.1: resolved "https://registry.npmjs.org/strip-json-comments/-/strip-json-comments-2.0.1.tgz"; integrity sha512-4gB8na07fecVVkOI6Rs4e7T6NOTki5EmL7TUduTs6bu3EdnSycntVJ4re8kgZA+wx9IueI2Y11bfbgwtzuE0KQ== +strnum@^1.0.5: Review Comment: Same here, this is MIT and needs to be in the LICENSE file. ########## src/xmlUtils/index.ts: ########## @@ -0,0 +1,69 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { XMLParser } from 'fast-xml-parser' + +function getAttributeValues( Review Comment: This function feels like it's adding generality but at the cost of added complexity. I'm wondering if that complexity is needed, and if readability woudl go up if replaced. For example, if queryXml instead just called parser.parse() and returned the resulting object, then the callers look like this (assuming you set the option to remove namespace prefixes): ```js let rootNamespace = obj["schema"]["@_targetNamespace"] ``` ```js let rootElems = obj["schema"]["element"].map((e) => e["@_name"])) ``` To me those lines feel more clear about how and what fields they are accessing. You might also need to add a try/catch around those lines handle if those keys don't exist, but that's doesn't feel too bad, and it makes the behavior more clear when they don't exist. Note that if you add this option to the `XMLParser`, you can make it so the "element" key is always an array and you don't have to do isArray checks: ```js isArray: (name, jpath, isLeafNode, isAttribute) => name == "element" ``` ########## yarn.lock: ########## @@ -2087,6 +2087,13 @@ fast-redact@^3.1.1: resolved "https://registry.npmjs.org/fast-redact/-/fast-redact-3.3.0.tgz"; integrity sha512-6T5V1QK1u4oF+ATxs1lWUmlEk6P2T9HqJG3e2DnHOdVgZy2rFJBoEnrIedcTXlkAHU/zKC+7KETJ+KGGKwxgMQ== +fast-xml-parser@^4.5.2: Review Comment: This is MIT licensed and requires an update to the LICENSE files. ASF cares heavily about licenses compatibility and proper attribution. Please make sure to stay on top of this. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
