-
Notifications
You must be signed in to change notification settings - Fork 22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix to remove dfdl prefix for dfdl attributes if the element containi… #1197
base: main
Are you sure you want to change the base?
Conversation
package.json
Outdated
"webpack": "webpack --mode production --config ./webpack/ext-dev.webpack.config.js", | ||
"webpack": "webpack --mode development --config ./webpack/ext-dev.webpack.config.js", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this intentional? If so, why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When debugging typescript code, hovering over variables to see their value doesn't work when webpack mode is set to production. Also variable names in the debug pane show as a, b, c, etc. instead of their actual name when webpack is set to production. I can change it back to production, but I will need to change it locally any time I want to debug typescript code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should keep using production mode for our builds. There may be some technique using environment variables or something so you don't need to keep changing it locally. It'll take a bit of research.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with using production mode for builds and figuring out a method for having the mode change to development when debugging locally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If webpack
is only used for development things, then I feel like it's fine to keep the mode as development.
I see there is a testing checklist as part of this PR. If we're supposed to follow that when reviewing this change, please mention that in the PR description. |
2b7b6de
to
b946931
Compare
I added a line mentioning the checklist to the description. |
package.json
Outdated
"webpack": "webpack --mode production --config ./webpack/ext-dev.webpack.config.js", | ||
"webpack": "webpack --mode development --config ./webpack/ext-dev.webpack.config.js", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with using production mode for builds and figuring out a method for having the mode change to development when debugging locally.
@@ -49,10 +50,36 @@ const items = [ | |||
'import', | |||
] | |||
|
|||
export class XmlItem { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is an XmlItem a representation of a whole XML tag (<tag attr="attrval"...>) or an attribute or both?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It appears to be an XML Element. I think it ought to be renamed to something a bit more descriptive and a bit less generic. Maybe XmlElement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can be an XML element tag or an XML attribute. It does not represent an entire element definition. I think naming the class XmlElement would not be the correct description. The class is defined in src/language/providers/utils.ts. It is used to store and retrieve the namespace prefix and the name of the element or attribute.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Include a comment of examples of what XmlItem can represent? Like xs:element
or dfdl:someAttribute
? What about cases there there isn't a namespace for elements like <body></body>
for example?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand your question, body isn't a dfdl element.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An XmlItem that doesn't have a namespace? I'm guessing it would be "none"
from the code, but wouldn't it represent none:<whatever goes here>
? Would it be better presented as private _itemNS:string | undefined = undefined
? If it's undefined
, it would mean the xml item doesn't belong to a namespace?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The XmlItem was introduced to keep track of the an element/attribute name and namespace. The use of none as the default values is to keep the returned results compatible with the existing code. There are various places where a return value of "none" changes the logic flow. I didn't want to re-work these situations with this change, (I was trying to get the changes in for an end of February release) but it can be looked at in future changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: Correct me if I'm wrong, but the files under src/language/providers provide functionality for auto suggestions or completions? It also looks like it provides logic for determining when to show the completions or not that relate to DFDL and what suggestions to show?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Those TS files mostly apply to DFDL autocompletions and suggestions. Also supplies auto completion for attribute values and for changing attribute values. And provides hover definitions for attributes. Some code for TDML completion is also in those TS files.
b946931
to
a75ea76
Compare
9a64c0d
to
bb44853
Compare
@@ -49,10 +50,36 @@ const items = [ | |||
'import', | |||
] | |||
|
|||
export class XmlItem { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An XmlItem that doesn't have a namespace? I'm guessing it would be "none"
from the code, but wouldn't it represent none:<whatever goes here>
? Would it be better presented as private _itemNS:string | undefined = undefined
? If it's undefined
, it would mean the xml item doesn't belong to a namespace?
…ng the attributes is a dfdl element Changed _XmlItem class name to XmlItem Changed changed XmlItem variable name to xmlItem Added dfdl intellisense checklist Added intellisense test dfdl schema files to src/tests/data Corrected line number references in intellisense testing checklist Closes apache#1080 Closes apache#1081 Closes apache#1170 Closes apache#1196
bb44853
to
e065a9b
Compare
…ng the
attributes is a dfdl element
Add dfdl intellisense checklist
Add intellisense test dfdl schema files to src/tests/data
The testing checklist src/tests/DfdlIntellisenseTestingChecklist.md can be used to verify that intellisense continues to work as expected.
Closes #1080
Closes #1081
Closes #1170
Closes #1196