-
Notifications
You must be signed in to change notification settings - Fork 4
NEW: @W-19397375@: Preserve working folders on error #355
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
Conversation
77f3d40 to
f983481
Compare
f983481 to
af6e283
Compare
| this.validateEnvironment(nodeVersion); | ||
| this.config = config; | ||
| this.tempFolder = new TempFolder(fileSystem); | ||
| /* istanbul ignore next */ |
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.
Could you clarify why this needed the ignore?
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 callback is really what's ignored. The test could never measure this because the node process is on exit when the call back is fired. So no way to cover this with a unit test. Demo (end-to-end) shows things working though.
| const describeOptions: engApi.DescribeOptions = { | ||
| workspace: engApiWorkspace, | ||
| workingFolder: await this.tempFolder.createSubfolder(describeWorkingFolderName, engineName), | ||
| workingFolder: workingFolder, |
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.
| workingFolder: workingFolder, | |
| workingFolder, |
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.
Nice cleanup 😎
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 actually find that this syntax is super confusing and it ties the variable name to the field name. So I personally choose not to use this type of syntax from javascript typically. Just a personal preference.
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.
For example, if I use my editor to rename a variable - it causes issues if I have "workingFolder" as both the fieldname and the variable. I might not want to change the field name... but may want to change the variable name and editors don't know this typically and get it wrong. Sometimes convenience and short code causes bugs... which is why I love static typing as well. :-)
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.
Works for me!
| export interface TempFolder { | ||
| getPath(): Promise<string>; | ||
| createSubfolder(...subfolderPathSegs: string[]): Promise<string>; | ||
| export interface FileSystem { |
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.
This is good - I feel like it could use elevating out of utils.ts
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.
But it is an implementation detail utility in my opinion.
randi274
left a comment
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.
This will be great to for us to add for some additional debugging support!
No description provided.