You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@daffodil.apache.org by GitBox <gi...@apache.org> on 2022/09/13 13:50:28 UTC
[GitHub] [daffodil-vscode] stevedlawrence commented on a diff in pull request #292: Added function to determine prefix in schema definition. Added calls …
stevedlawrence commented on code in PR #292:
URL: https://github.com/apache/daffodil-vscode/pull/292#discussion_r969570126
##########
src/language/providers/utils.ts:
##########
@@ -251,46 +316,72 @@ export function checkSetVariableOpen(
return false
}
+export function getNsPrefix(
+ document: vscode.TextDocument,
+ position: vscode.Position
+) {
+ var nsPrefix = ''
Review Comment:
Maybe this is just me being used to scala, but this mutable nsPrefix variable seems unnecessary. We never really mutate this variable except in the `<.*:schema` case, but then we immediately return `nsPrefix`, so mutating doesn't really do anything. Seems it would be more clear to just return empty string in two of the cases, and `return lineText...` in the other case.
##########
src/language/providers/utils.ts:
##########
@@ -251,46 +316,72 @@ export function checkSetVariableOpen(
return false
}
+export function getNsPrefix(
Review Comment:
Can we rename this something like `getXsdNsPrefix` or something, just to make it more clear that his will only return the prefix for that namespace?
Also, it looks like this includes the colon if there is a prefix. This definitely makes a lot of the code above cleaner, but I think it's worth a comment. That this either returns empty string if there is no prefix, or the prefix + colon if there is a refix.
##########
src/language/providers/utils.ts:
##########
@@ -251,46 +316,72 @@ export function checkSetVariableOpen(
return false
}
+export function getNsPrefix(
+ document: vscode.TextDocument,
+ position: vscode.Position
+) {
+ var nsPrefix = ''
+ var initialLineNum = position.line
+ var lineNum = 0
+ while (initialLineNum !== 0) {
+ const lineText = document
+ .lineAt(0)
+ .text.substr(0, document.lineAt(lineNum).range.end.character)
+ if (lineText.includes('<schema')) {
+ return nsPrefix
+ }
+ if (lineText.match(new RegExp('<.*:schema'))) {
Review Comment:
`.*` is greedy so there could be some edge cases where it matches too much, for example:
```xml
<xs:schema xmlns:xs="..." xmlns:urn="foo:bar:schema" />
```
Instead we should use something like `[^ ]+` so it matches only non-space characters. Also, I think we can use a single regex to match both cases, and a capture group to get the matched prefix, which simplifies things a bit. For example:
```js
var prefixRegex = new Regex('<(|[^ ]+:)schema')
...
while(...) {
const lineText = document.lineAt(lineNum)
var prefixRegexMatch = prefixRegex.exec(lineText)
if (prefixRegexMatch != null) {
return prefixRegexMatch[1]
}
...
}
```
The regex will match either an emtpy string followed by schema, or one or more non-space charcters followed by a colon followed by schema. And the prefix (including colon) is part of the capture group.
This isn't tested, but that's the idea.
##########
src/language/providers/utils.ts:
##########
@@ -251,46 +316,72 @@ export function checkSetVariableOpen(
return false
}
+export function getNsPrefix(
+ document: vscode.TextDocument,
+ position: vscode.Position
+) {
+ var nsPrefix = ''
+ var initialLineNum = position.line
+ var lineNum = 0
+ while (initialLineNum !== 0) {
+ const lineText = document
+ .lineAt(0)
+ .text.substr(0, document.lineAt(lineNum).range.end.character)
Review Comment:
This doesn't seem right. For every iteration of this loop this gets the contents of file from the 0th line to lineNum, that shouldn't be needed. Can this just be
```js
const lineText = document.lineAt(lineNum)
```
##########
src/language/providers/attributeCompletion.ts:
##########
@@ -266,21 +274,28 @@ export function getAttributeCompletionProvider() {
)
}
-function getDefinedTypes(document: vscode.TextDocument) {
+function getDefinedTypes(
+ document: vscode.TextDocument,
+ position: vscode.Position
+) {
var additionalTypes = ''
var lineNum = 0
const lineCount = document.lineCount
+ // const position = new vscode.Position(
+ // lineCount,
+ // document.lineAt(lineCount).range.end.character)
+ const nsPrefix = getNsPrefix(document, position)
Review Comment:
We call `getNSPrefix` quite a bit. Can it be called once per autocomplete instantiation and shared mong all the calls so we aren't constantly scanning for the schema tag? Or maybe we already do taht? It's not clear to me what the entrypoint into autocomplete is.
##########
src/language/providers/attributeCompletion.ts:
##########
@@ -75,31 +76,38 @@ export function getAttributeCompletionProvider() {
document: vscode.TextDocument,
position: vscode.Position
) {
- const wholeLine = document
+ const triggerText = document
.lineAt(position)
.text.substr(0, position.character)
+ var nsPrefix = getNsPrefix(document, position)
var nearestOpenItem = nearestOpen(document, position)
if (
!checkBraceOpen(document, position) &&
- !wholeLine.includes('assert') &&
+ !triggerText.includes('assert') &&
!nearestOpenItem.includes('none')
) {
if (nearestOpenItem.includes('element')) {
var preVal = ''
- if (!wholeLine.includes('xs:element')) {
+ if (!triggerText.includes('<' + nsPrefix + 'element')) {
if (lineCount(document, position) === 1) {
preVal = '\t'
} else {
preVal = ''
}
}
- var additionalItems = getDefinedTypes(document)
+ var additionalItems = getDefinedTypes(document, position)
+ // if (
+ // checkLastItemOpen(document, position) &&
+ // (triggerText.includes('<xs:element name="') ||
+ // triggerText.includes('<xs:element ref="') ||
+ // checkElementOpen(document, position))
+ // ) {
Review Comment:
Why was this commented out? I would prefer we don't change any behavior and limit this PR to only namespace changes.
##########
src/language/providers/intellisense/elementItems.ts:
##########
@@ -29,84 +29,84 @@ export const elementCompletion = (definedVariables, dfdlFormatString) => {
},
{
item: 'xs:element name',
- snippetString: '<xs:element name="$1"$0',
+ snippetString: '<' + nsPrefix + 'element name="$1"$0',
markdownString: 'A new xs element',
},
{
item: 'xs:element ref',
- snippetString: '<xs:element ref="$1"$0',
+ snippetString: '<' + nsPrefix + 'element ref="$1"$0',
markdownString: 'A new dfdl reference to an item',
},
{
item: 'xs:group name',
- snippetString: '<xs:group name = "$1">\n\t$0\n</xs:group>',
+ snippetString: '<' + nsPrefix + 'group name = "$1">\n\t$0\n</' + nsPrefix + 'group>',
},
{
item: 'xs:group ref',
- snippetString: '<xs:group ref="$1"$0',
+ snippetString: '<' + nsPrefix + 'group ref="$1"$0',
markdownString: 'A new dfdl reference to an item',
},
{
item: 'dfdl:assert',
- snippetString: '<xs:annotation>\n\t<xs:appinfo source="http://www.ogf.org/dfdl/">\n\t\t<dfdl:assert>"<$1>"</dfdl:assert>\n\t</xs:appinfo>\n</xs:annotation>$0',
+ snippetString: '<' + nsPrefix + 'annotation>\n\t<' + nsPrefix + 'appinfo source="http://www.ogf.org/dfdl/">\n\t\t<dfdl:assert>"<$1>"</dfdl:assert>\n\t</' + nsPrefix + 'appinfo>\n</' + nsPrefix + 'annotation>$0',
markdownString: 'dfdl assertion test',
},
{
item: 'dfdL:discriminator',
- snippetString: '<xs:annotation>\n\t<xs:appinfo source="http://www.ogf.org/dfdl/">\n\t\t<dfdl:discriminator test="{$1}"/>\n\t</xs:appinfo>\n</xs:annotation>$0',
+ snippetString: '<' + nsPrefix + 'annotation>\n\t<' + nsPrefix + 'appinfo source="http://www.ogf.org/dfdl/">\n\t\t<dfdl:discriminator test="{$1}"/>\n\t</' + nsPrefix + 'appinfo>\n</' + nsPrefix + 'annotation>$0',
markdownString: 'dfdl discriminator test',
},
{
item: 'dfdl:hiddenGroupRef',
- snippetString: '<xs:sequence dfdl:hiddenGroupRef="$1"/>\n$0',
+ snippetString: '<' + nsPrefix + 'sequence dfdl:hiddenGroupRef="$1"/>\n$0',
},
{
item: 'dfdl:format',
snippetString: dfdlFormatString,
},
{
item: 'xs:annotation',
- snippetString: '<xs:annotation>\n\t<xs:appinfo source="http://www.ogf.org/dfdl/">\n\t\t$0\n\t</xs:appinfo>\n</xs:annotation>',
+ snippetString: '<' + nsPrefix + 'annotation>\n\t<' + nsPrefix + 'appinfo source="http://www.ogf.org/dfdl/">\n\t\t$0\n\t</' + nsPrefix + 'appinfo>\n</' + nsPrefix + 'annotation>',
},
{
item: 'xs:appinfo',
- snippetString: '<xs:annotation>\n\t<xs:appinfo source="http://www.ogf.org/dfdl/">\n\t\t$0\n\t</xs:appinfo>\n</xs:annotation>',
+ snippetString: '<' + nsPrefix + 'annotation>\n\t<' + nsPrefix + 'appinfo source="http://www.ogf.org/dfdl/">\n\t\t$0\n\t</' + nsPrefix + 'appinfo>\n</' + nsPrefix + 'annotation>',
},
{
item: 'xs:complexType',
- snippetString: '<xs:complexType>\n\t$0\n</xs:complexType>',
+ snippetString: '<' + nsPrefix + 'complexType>\n\t$0\n</' + nsPrefix + 'complexType>',
markdownString: 'Starts a complex type definition',
},
{
item: 'xs:complexType name=',
- snippetString: '<xs:complexType Name="$1">\n\t$0\n</xs:complexType>',
+ snippetString: '<' + nsPrefix + 'complexType Name="$1">\n\t$0\n</' + nsPrefix + 'complexType>',
markdownString: 'Starts a complex type definition',
},
{
item: 'xs:simpleType',
- snippetString: '<xs:simpleType>\n\t$0\n</xs:simpleType>',
+ snippetString: '<' + nsPrefix + 'simpleType>\n\t$0\n</' + nsPrefix + 'simpleType>',
markdownString: 'Starts a simple type definition',
},
{
item: 'xs:simpleType name=',
- snippetString: '<xs:simpleType Name="$1"$0',
+ snippetString: '<' + nsPrefix + 'simpleType Name="$1"$0',
markdownString: 'Starts a simple type definition',
},
{
item: 'xs:sequence',
- snippetString: '<xs:sequence',
+ snippetString: '<' + nsPrefix + 'sequence',
},
{
item: 'xs:choice',
- snippetString: '<xs:choice$1>\n\t$0\n</xs:choice>',
+ snippetString: '<' + nsPrefix + 'choice',
Review Comment:
More changes that aren't prefix related. This actually changes what is autocompleted to not add close tag. That should be done as a separate PR which a separate issue if that's what we want.
##########
src/language/providers/attributeCompletion.ts:
##########
@@ -75,31 +76,38 @@ export function getAttributeCompletionProvider() {
document: vscode.TextDocument,
position: vscode.Position
) {
- const wholeLine = document
+ const triggerText = document
Review Comment:
Is this `triggerText` rename needed for the prefix stuff? Seems like that's unrelated and should be fixed as #165. This would be much easier to review without all the rename changes. It makes it difficult to pick out the parts that are relevant to the ns changes.
##########
src/language/providers/intellisense/elementItems.ts:
##########
@@ -29,84 +29,84 @@ export const elementCompletion = (definedVariables, dfdlFormatString) => {
},
{
Review Comment:
Should we change the `xs:schema` autocomplete above to be opinionated about whether or not it uses the xs prefix? I think we're starting to move away from that prefix? We can do that in a separate PR, but it's worth considering doing now that we can support different prefixes. We may also want to reconsider some of these other things in the xs:schema tag. For example, I think we're also moving towards `elementFormDefault=unqualified`, and I'm not sure if xmlns:daf is used anywhere anymore--I believe that namespace has been deprecated.
##########
src/language/providers/utils.ts:
##########
@@ -187,20 +222,50 @@ export function checkSequenceOpen(
return false
}
+export function checkChoiceOpen(
Review Comment:
This change was added as part of #283, right? Please don't include changes from other PR's in new PRs. I've merged that PR, btw.
##########
src/language/providers/attributeCompletion.ts:
##########
@@ -266,21 +274,28 @@ export function getAttributeCompletionProvider() {
)
}
-function getDefinedTypes(document: vscode.TextDocument) {
+function getDefinedTypes(
+ document: vscode.TextDocument,
+ position: vscode.Position
+) {
var additionalTypes = ''
var lineNum = 0
const lineCount = document.lineCount
+ // const position = new vscode.Position(
+ // lineCount,
+ // document.lineAt(lineCount).range.end.character)
Review Comment:
Remove comment.
##########
src/language/providers/closeElement.ts:
##########
@@ -27,59 +27,71 @@ export function getCloseElementProvider() {
position: vscode.Position
) {
var backpos = position.with(position.line, position.character - 1)
+ const nsPrefix = getNsPrefix(document, position)
const nearestOpenItem = nearestOpen(document, position)
- const wholeLine = document
+ const triggerText = document
.lineAt(position)
.text.substr(0, position.character)
if (
- wholeLine.endsWith('>') &&
- (wholeLine.includes('xs:element') ||
+ !triggerText.includes('</') &&
Review Comment:
This is a change not related to prefix stuff I think? Please try to keep unrelated bug fixes separate.
##########
src/language/providers/utils.ts:
##########
@@ -251,46 +316,72 @@ export function checkSetVariableOpen(
return false
}
+export function getNsPrefix(
+ document: vscode.TextDocument,
+ position: vscode.Position
+) {
+ var nsPrefix = ''
+ var initialLineNum = position.line
+ var lineNum = 0
+ while (initialLineNum !== 0) {
+ const lineText = document
+ .lineAt(0)
+ .text.substr(0, document.lineAt(lineNum).range.end.character)
+ if (lineText.includes('<schema')) {
+ return nsPrefix
+ }
+ if (lineText.match(new RegExp('<.*:schema'))) {
+ nsPrefix = lineText.substring(
+ lineText.indexOf('<') + 1,
+ lineText.indexOf(':') + 1
+ )
+ return nsPrefix
+ }
+ ++lineNum
+ }
+ return nsPrefix
+}
Review Comment:
I'm not sure how important this is, but this function could do the wrong thing if there's a schema tag in a comment. For example:
```xml
<!--
this is some comment that for some reason contains an xs:schema tag,
maybe some kind of documentation?
<xs:schema xmlns:xs="..." >
-->
<schema xmlns="..." ...>
```
In this case, this function will return the `xs` prefix because this function doesn't know to ignore comments. Again, I'm not sure if that's a big deal, seems like a pretty rare edge case and maybe not worth added complexity of parsing XML looking for comments.
##########
src/language/providers/utils.ts:
##########
@@ -63,13 +79,13 @@ export function lineCount(
while (lineNum !== 0) {
--lineNum
++lineCount
- const wholeLine = document
+ const triggerText = document
.lineAt(lineNum)
.text.substr(0, document.lineAt(lineNum).range.end.character)
if (
- wholeLine.includes('<xs:element') &&
- !wholeLine.includes('</xs:element') &&
- !wholeLine.includes('/>')
+ triggerText.match(new RegExp('<.*element')) &&
+ !triggerText.match(new RegExp('</.*element')) &&
+ !triggerText.includes('/>')
Review Comment:
This is fragile, `.*` can match way too much text because of its greediness. Can you use the `"<" + nsPrefix + "element"` that you've used elsewhere?
##########
src/language/providers/utils.ts:
##########
@@ -251,46 +316,72 @@ export function checkSetVariableOpen(
return false
}
+export function getNsPrefix(
+ document: vscode.TextDocument,
+ position: vscode.Position
+) {
+ var nsPrefix = ''
+ var initialLineNum = position.line
+ var lineNum = 0
+ while (initialLineNum !== 0) {
Review Comment:
initialLineNum never changes, this isn't a good condition for breaking the loop. If a match is never found (e.g. and empty file) then this will be stuck in an infinite loop.
##########
src/language/providers/utils.ts:
##########
@@ -25,31 +25,47 @@ export function insertSnippet(snippetString: string, backpos: vscode.Position) {
)
}
-//Checks if the line at the current position is the last opened tag
+//Checks if the text at the current position is the last opened tag
export function checkLastItemOpen(
document: vscode.TextDocument,
position: vscode.Position
) {
var lineNum = position.line
- const wholeLine = document
+ const triggerText = document
.lineAt(lineNum)
.text.substr(0, document.lineAt(lineNum).range.end.character)
- while (wholeLine.length === 0) {
+ while (triggerText.length === 0) {
--lineNum
}
- const previousLine = document
- .lineAt(lineNum)
- .text.substr(0, document.lineAt(lineNum - 1).range.end.character)
- if (
- previousLine.includes('</') ||
- previousLine.includes('/>') ||
- ((wholeLine.includes('element') ||
- wholeLine.includes('sequence') ||
- wholeLine.includes('group') ||
- wholeLine.includes('Variable')) &&
- (wholeLine.includes('</') || wholeLine.includes('/>')))
- ) {
- return false
+ if (lineNum > 0) {
Review Comment:
I'm not sure I understand what this change is? Doesn't seem related to prefixes.
##########
src/language/providers/closeElement.ts:
##########
@@ -27,59 +27,71 @@ export function getCloseElementProvider() {
position: vscode.Position
) {
var backpos = position.with(position.line, position.character - 1)
+ const nsPrefix = getNsPrefix(document, position)
const nearestOpenItem = nearestOpen(document, position)
- const wholeLine = document
+ const triggerText = document
.lineAt(position)
.text.substr(0, position.character)
if (
- wholeLine.endsWith('>') &&
- (wholeLine.includes('xs:element') ||
+ !triggerText.includes('</') &&
+ triggerText.endsWith('>') &&
+ (triggerText.includes('<' + nsPrefix + 'element') ||
nearestOpenItem.includes('element') ||
- wholeLine.includes('xs:group') ||
+ triggerText.includes('<' + nsPrefix + 'group') ||
nearestOpenItem.includes('group') ||
- wholeLine.includes('xs:sequence') ||
+ triggerText.includes('<' + nsPrefix + 'sequence') ||
nearestOpenItem.includes('sequence') ||
- wholeLine.includes('xs:simpleType') ||
+ triggerText.includes('<' + nsPrefix + 'simpleType') ||
nearestOpenItem.includes('simpleType') ||
- wholeLine.includes('dfdl:defineVariable') ||
+ triggerText.includes('<' + nsPrefix + 'choice') ||
+ nearestOpenItem.includes('choice') ||
+ triggerText.includes('dfdl:defineVariable') ||
nearestOpenItem.includes('Variable'))
) {
var range = new vscode.Range(backpos, position)
vscode.window.activeTextEditor?.edit((editBuilder) => {
editBuilder.replace(range, '')
})
if (
- wholeLine.endsWith('>') &&
- (wholeLine.includes('xs:element ref') ||
- wholeLine.includes('xs:group ref'))
+ triggerText.endsWith('>') &&
+ (triggerText.includes('<' + nsPrefix + 'element ref') ||
+ triggerText.includes('<' + nsPrefix + 'group ref'))
) {
insertSnippet(' />\n$0', backpos)
} else if (
- wholeLine.endsWith('>') &&
- (wholeLine.includes('xs:element') ||
+ triggerText.endsWith('>') &&
+ (triggerText.includes('<' + nsPrefix + 'element') ||
nearestOpenItem.includes('element'))
) {
- insertSnippet('>\n\t$0\n</xs:element>', backpos)
+ insertSnippet('>\n\t$0\n</' + nsPrefix + 'element>', backpos)
} else if (
- wholeLine.endsWith('>') &&
- (wholeLine.includes('xs:group') ||
+ triggerText.endsWith('>') &&
+ (triggerText.includes('<' + nsPrefix + 'group') ||
nearestOpenItem.includes('group'))
) {
- insertSnippet('>\n\t$0\n</xs:group>', backpos)
+ insertSnippet('>\n\t$0\n</' + nsPrefix + 'group>', backpos)
} else if (
- (wholeLine.endsWith('>') && wholeLine.includes('xs:sequence')) ||
+ (triggerText.endsWith('>') &&
+ triggerText.includes('<' + nsPrefix + 'sequence')) ||
nearestOpenItem.includes('sequence')
) {
- insertSnippet('>\n\t$0\n</xs:sequence>', backpos)
+ insertSnippet('>\n\t$0\n</' + nsPrefix + 'sequence>', backpos)
} else if (
- (wholeLine.endsWith('>') && wholeLine.includes('xs:simpleType')) ||
+ (triggerText.endsWith('>') &&
+ triggerText.includes('<' + nsPrefix + 'choice')) ||
+ nearestOpenItem.includes('choice')
Review Comment:
I think this is part of the choice PR that was just merged?
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org