fix: show error message when xml-model exists but href is missing#49
fix: show error message when xml-model exists but href is missing#49mahithakkar wants to merge 2 commits intoraffazizzi:mainfrom
Conversation
There was a problem hiding this comment.
Reading through, I made a few comments addressing possible optimizations and style concerns that came to mind. It's possible for these choices to have a reason, but I'll leave it to you to read my observations and make a decision from there.
Solid Work! :)
| const hasXmlModel = fileText.match(/<\?xml-model/); | ||
| if (hasXmlModel && !schemaURLMatch) { | ||
| console.log("Found xml-model but no href!"); | ||
| window.showInformationMessage("Schema not associated correctly — make sure you're using href= in your <?xml-model?>"); |
There was a problem hiding this comment.
Explanatory error messages are nice, and a part of user-centered designing and user-experience!
|
|
||
| if (schemaURLMatch) schemaURL = schemaURLMatch[1]; | ||
|
|
||
| //this function gets the full text of xml file, then searches for the model and href in it. |
There was a problem hiding this comment.
I noticed some longer comments here, and while they help me understand the code better, I wish they were visible at the top of the function.
Larger comments describing the function at high-level are typically useful to read beforehand because the give an intro to the logic without interrupting readability.
If you prefer in-line comments however, I think they would be more readable in split segments, describing line by line.
|
|
||
| //this function takes the full document text as a string and returns an arrays with all the | ||
| //IDs that it found. | ||
| export function collectXmlIds(documentText: string): string[] { |
There was a problem hiding this comment.
The collectXmlIds() function is well explained with in-line comments as well as high level explanations, that allowed me to follow the logic and understand the greater purpose.
| }); | ||
|
|
||
| try { | ||
| parser.write(documentText).close(); |
There was a problem hiding this comment.
I noticed the try catch expression, and it caught my attention seeing as it is never handled.
Reading the comments, I see that this is intentional, but I wonder if there should still be some type of status displayed to the user still, like a debug log.
| attr.local === "id" && | ||
| attr.uri === "http://www.w3.org/XML/1998/namespace" | ||
| ) { | ||
| if (attr.value && !ids.includes(attr.value)) { |
There was a problem hiding this comment.
This line does a good job not ensuring duplicates, but !ids.includes(attr.value) is computationally O(n). This leads the function to be O(n^2) when it might not need to be.
There should be ways to avoid that extra computation and ensure that there are no repeated id added, but one that comes to mind is using a Set().
ids = new Set<string>();
...
ids.add(attr.value);
...
return Array.from(ids);
Stepping back , I wonder if a message or status should let users know if multiple ids are repeating. To my knowledge, id’s are supposed to be unique, but if that is handled somewhere else you can safely ignore that thought.
There was a problem hiding this comment.
This is a good point. Repeated IDs are flagged for the users elsewhere and I think here they should just be added to the array for two reasons. 1) avoid unnecessary complexity like @nolawetemketem mentioned, and 2) be honest to the document: if there are repeated IDs, they should show up in this list as duplicates, too
| @@ -1,5 +1,5 @@ | |||
| import { CompletionRequest } from "../constants"; | |||
| import { truncate } from "../utils"; | |||
| import { truncate , collectXmlIds} from "../utils"; | |||
There was a problem hiding this comment.
I understand suggest.ts isn't just your code, but I have less understanding than other parts of the codebase because of the fewer comments, and I think thats an observation thats valuable to improving the code overall.
| const items: CompletionItem[] = []; | ||
| const xmlIds = collectXmlIds(documentText); | ||
| for (const id of xmlIds) { | ||
| const ci = new CompletionItem(`#${id}`, 24); |
There was a problem hiding this comment.
Reading for the first-time I dont understand what24 is representing.
I believe that using a variables or symbolic constants can improve readability for someone not as familiar with the codebase.
|
|
||
| return items; | ||
| if (request === CompletionRequest.VAL) { //only run this when user is typing an attribute value | ||
| const xmlIds = collectXmlIds(documentText); //get all xml:id values from the document |
There was a problem hiding this comment.
I noticed code block at lines 365 and 69. I wonder if it's possible to consolidate the logic into one function defined : function buildXmlIdCompletions(documentText: string): CompletionItem[].
Additionally, I wonder if is it necessary to execute collectXmlIds(documentText) in two different places for the functionality you want, as parsing documentText twice is computationally more expensive.
If thats apart of the extension’s design, great but If not, consider a way to parse it once.
There was a problem hiding this comment.
I agree this can be DRYer by at least merging the code blocks as @nolawetemketem suggested. It's likely that the function needs to be invoked twice (I think one triggers when typing '#', the other when the use specifically calls for completions (e.g. via ctrl+space), but worth double checking
3 participants
I worked on issue #36. The extension was stuck showing “Loading” forever when a user wrote but used file instead of href. My approach was to first check if the model line exists, and if it does, then check that href is present. If href is missing, we now show an error message so the user can better understand it.
I added a check in locateSchema in locate.ts that checks when there is an line in the file but it doesn't have a valid href=. Then shows a helpful popup message telling the user to check their about the missing href.
I couldn't fully verify the popup appearing in the Extension Development Host (the test XML file didn't have a real schema next to it so the extension wasn't triggering). However I used a different approach to test it. I created a quick temp file and ran it with Node.js directly in the terminal, which confirmed the check works correctly. It detects the missing href= and would trigger the error message.