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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
