-
Notifications
You must be signed in to change notification settings - Fork 295
Detect Visual Studio versions on the fly #8
Conversation
src/config.coffee
Outdated
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.
Would fs.existsSync work here instead? Seems similar.
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.
On Windows, it is possible (though unlikely in this case) that you could determine that a file exists but you can't actually read it (because of busted permissions, your computer hates you, etc).
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.
Yeah, but I thought node's fs.existsSync is implemented this same way: https://github.com/joyent/node/blob/master/lib/fs.js#L161-L169
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.
Legit. Replacing
src/installer.coffee
Outdated
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.
Need parens around isWin32
src/installer.coffee
Outdated
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 usually sport a little if vsArgs? instead of direct compares to 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.
TIL a cool CoffeeScript hack
|
@paulcbetts this PR seems nearly completed, is there any blocking problem? |
|
@zcbenz Think I just spaced on it. I'll get it checked in today. |
Detect Visual Studio versions on the fly
Atom build servers on Windows only have VS2010 installed. Support using either one, but prefer VS2012 if both are present.