-
Notifications
You must be signed in to change notification settings - Fork 451
Status cmd is working - still have a few TODOs #68
Conversation
User Story #884931
Hi @jpricketMSFT, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
TTYL, MSBOT; |
import { parseString } from "xml2js"; | ||
|
||
export class CommandHelper { | ||
public static async parseXml(xml: string): Promise<any> { |
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.
CAP-it
return name; | ||
} | ||
|
||
public static trimToXml(xml: string): string { |
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.
CAP-it
export class CommandHelper { | ||
public static async parseXml(xml: string): Promise<any> { | ||
return new Promise<any>((fulfill, reject) => { | ||
parseString(xml, { |
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'd push the { to the next line and align with the 'x' in xml, then...
attrNameProcessors: [CommandHelper.normalizeName] | ||
}, | ||
(err, result) => { | ||
if (err) { |
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.
Can you indent the "if (err) {" or is tslint down with it like this?
if (err) { | ||
reject(err); | ||
} else { | ||
fulfill(result); |
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'm not sure of the diff between fulfill and resolve. I use "resolve".
// Then, show the quick pick window and get back the one they chose | ||
//TODO localize strings | ||
let item: QuickPickItem = await window.showQuickPick( | ||
items, { matchOnDescription: true, placeHolder: "Choose a file to open the file." }); |
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.
At some point, we'll have to put these hard-coded strings into a Strings file.
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.
Agreed. Just trying to let the design settle a bit before doing that. Let's talk about how it should be done on Monday.
} | ||
} | ||
} | ||
return null; |
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.
How's about either undefined or void 0?
const json = await CommandHelper.parseXml(xml); | ||
if (json && json.status) { | ||
// get all the pending changes first | ||
const pending = json.status.pendingchanges[0].pendingchange; |
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 there a data type for pending?
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.
Do that, if you can, for all of your other variable definitions.
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.
Had to use any :(
User Story #884931
User Story #884931